Details

      Description

      Currently you can see stored passwords in HTML body of the page which is quite big security hole. We could rewrite it so that the field is presented with some predefined constant string, like "###########" (only to show the field with some entered text). Then in process*Post handlers we should check if someone entered anything different here and only in such case overwrite previously stored password. When posted value is equal to "###########" - we leave previous password in configuration intact.

      this applies to almost all connectors...

        Activity

        Hide
        Karl Wright added a comment -

        Hi Maciej,

        The current security model basically presumes that there is one user of the ManifoldCF UI, or at least that all users of the ManifoldCF UI have similar levels of authority as far as knowledge of passwords is concerned. Using the "password"-type HTML form element is simply to prevent people seeing passwords on the screen looking over somebody's shoulder.

        So I guess the point is that if we wanted a more robust model for user security, there are probably a lot of things we'd want to change. For example, you would probably want to grant individual users or user groups responsibility for each connection (since that's where the passwords are), etc. It seems like your fix is an overly narrow one, perhaps.

        Show
        Karl Wright added a comment - Hi Maciej, The current security model basically presumes that there is one user of the ManifoldCF UI, or at least that all users of the ManifoldCF UI have similar levels of authority as far as knowledge of passwords is concerned. Using the "password"-type HTML form element is simply to prevent people seeing passwords on the screen looking over somebody's shoulder. So I guess the point is that if we wanted a more robust model for user security, there are probably a lot of things we'd want to change. For example, you would probably want to grant individual users or user groups responsibility for each connection (since that's where the passwords are), etc. It seems like your fix is an overly narrow one, perhaps.
        Hide
        Maciej Lizewski added a comment -

        not exactly. when password are in concern we should pay maximum attention and reduce possibility of password leak to the minimum.
        I understand that we assume that access to Manifold is somehow protected and that there is only one level of security, so everyone that has access to all Manifold configuration. On the other hand - these are passwords allowing to read ALL DOCUMENTS in every repository! if Manifold administrator leave unlocked console - it is too easy to read them and then use them. It is generally not a good pattern to rely only on single authentication and granting any access to plain text passwords - this is why in systems like LDAP they are not even stored in database but only their hashes...
        I think proposed procedure is quite simple and easy and could prevent very serious accidents. Security is about reducing all possibilities of something goes wrong and we should tighten Manifold in this area.

        the only problem here is some amount of work we should do to change current approach...

        Show
        Maciej Lizewski added a comment - not exactly. when password are in concern we should pay maximum attention and reduce possibility of password leak to the minimum. I understand that we assume that access to Manifold is somehow protected and that there is only one level of security, so everyone that has access to all Manifold configuration. On the other hand - these are passwords allowing to read ALL DOCUMENTS in every repository ! if Manifold administrator leave unlocked console - it is too easy to read them and then use them. It is generally not a good pattern to rely only on single authentication and granting any access to plain text passwords - this is why in systems like LDAP they are not even stored in database but only their hashes... I think proposed procedure is quite simple and easy and could prevent very serious accidents. Security is about reducing all possibilities of something goes wrong and we should tighten Manifold in this area. the only problem here is some amount of work we should do to change current approach...
        Hide
        Karl Wright added a comment -

        Hi Maciej,

        There's all sorts of havoc an unauthorized user can wreak if an unauthorized user has access to the ManifoldCF UI. Getting access to passwords is just a small part of that. That was my point.

        The current recommendation is therefore that access to the ManifoldCF UI be restricted to only the people who have access to such passwords. If that's not possible then your proposal isn't going to help much.

        I'm not opposed to starting the process of tightening up the UI so that it is secure in an open environment, but please do understand that it is just the first of many steps.

        As for the approach you propose, I would like to think through a canonical implementation. As you point out, it affects nearly every connector, and therefore we want to do it right in one connector first, before we apply it everywhere. If we go with a special token indicating "password unchanged", then we should make sure that the password includes special Unicode characters that are unlikely to come up in real passwords, and we should standardize this special string by maybe putting it in the BaseConnector classes as a constant.

        Show
        Karl Wright added a comment - Hi Maciej, There's all sorts of havoc an unauthorized user can wreak if an unauthorized user has access to the ManifoldCF UI. Getting access to passwords is just a small part of that. That was my point. The current recommendation is therefore that access to the ManifoldCF UI be restricted to only the people who have access to such passwords. If that's not possible then your proposal isn't going to help much. I'm not opposed to starting the process of tightening up the UI so that it is secure in an open environment, but please do understand that it is just the first of many steps. As for the approach you propose, I would like to think through a canonical implementation. As you point out, it affects nearly every connector, and therefore we want to do it right in one connector first, before we apply it everywhere. If we go with a special token indicating "password unchanged", then we should make sure that the password includes special Unicode characters that are unlikely to come up in real passwords, and we should standardize this special string by maybe putting it in the BaseConnector classes as a constant.
        Hide
        Maciej Lizewski added a comment -

        ok, so lets do like this: create canonical implementation and functionality leaving current connectors as they are. New connectors and changes will be encouraged to use this new mechanism. what do you think? we could even postpone this issue to next realeases, but I would be glad not to see clear passwords

        Show
        Maciej Lizewski added a comment - ok, so lets do like this: create canonical implementation and functionality leaving current connectors as they are. New connectors and changes will be encouraged to use this new mechanism. what do you think? we could even postpone this issue to next realeases, but I would be glad not to see clear passwords
        Hide
        Karl Wright added a comment -

        Hi Maciej,

        This sounds like a fine approach, although I'd identify ONE current connector and do an implementation that meets the goals. Then we can do the rest as time permits, and also encourage new connectors to use the new approach.

        Show
        Karl Wright added a comment - Hi Maciej, This sounds like a fine approach, although I'd identify ONE current connector and do an implementation that meets the goals. Then we can do the rest as time permits, and also encourage new connectors to use the new approach.
        Hide
        Karl Wright added a comment -

        I am triaging it for 1.3, although I don't think all connectors will be done then.

        Show
        Karl Wright added a comment - I am triaging it for 1.3, although I don't think all connectors will be done then.
        Hide
        Karl Wright added a comment -

        I will create a branch after HDFS connector is merged.

        Show
        Karl Wright added a comment - I will create a branch after HDFS connector is merged.
        Hide
        Maciej Lizewski added a comment -

        sounds great! I know you have lots of other work to do before 1.3 so I appreciate this. I think when there will be sample implementation - other committers can help improving their connectors (at least I am going to do that )

        Show
        Maciej Lizewski added a comment - sounds great! I know you have lots of other work to do before 1.3 so I appreciate this. I think when there will be sample implementation - other committers can help improving their connectors (at least I am going to do that )
        Hide
        Karl Wright added a comment -

        r1497409 tries this out with the GoogleDrive connector.

        Show
        Karl Wright added a comment - r1497409 tries this out with the GoogleDrive connector.
        Hide
        Maciej Lizewski added a comment -

        this is exactly I was talking about!

        Show
        Maciej Lizewski added a comment - this is exactly I was talking about!
        Hide
        Karl Wright added a comment -

        r1498905 does Alfresco, CMIS, Documentum, and Dropbox connectors.

        Show
        Karl Wright added a comment - r1498905 does Alfresco, CMIS, Documentum, and Dropbox connectors.
        Hide
        Karl Wright added a comment -

        r1500263 does the remaining connectors except for Active Directory and Web.

        Show
        Karl Wright added a comment - r1500263 does the remaining connectors except for Active Directory and Web.
        Hide
        Karl Wright added a comment -

        This strategy won't work. It has the main problem that there are times where the ONLY password value being tracked is in fact in the posted data, and there is no other copy, e.g. new connections where there are multiple tabs. There's also a problem of synchronization between multiple sessions, which will someday be a blocker as well.

        After some consideration, I believe that this issue can only properly be solved by doing two things:

        (a) Introduce sessions into the MCF crawler UI;
        (b) Put existing values of passwords into session variables, in a map where there's a key (which is actually what gets posted), and the current value.

        Show
        Karl Wright added a comment - This strategy won't work. It has the main problem that there are times where the ONLY password value being tracked is in fact in the posted data, and there is no other copy, e.g. new connections where there are multiple tabs. There's also a problem of synchronization between multiple sessions, which will someday be a blocker as well. After some consideration, I believe that this issue can only properly be solved by doing two things: (a) Introduce sessions into the MCF crawler UI; (b) Put existing values of passwords into session variables, in a map where there's a key (which is actually what gets posted), and the current value.
        Hide
        Karl Wright added a comment -

        r1500409 reverts all changes for CONNECTORS-737, until such time as we can do this the right way.

        Show
        Karl Wright added a comment - r1500409 reverts all changes for CONNECTORS-737 , until such time as we can do this the right way.
        Hide
        Karl Wright added a comment -

        The strategy I will use to fix this is as follows:

        (a) Write a class that is referenced by the session bean, which mints password replacement values, and knows how to revert them. API:

        String convert(String passwordValue)
        String revert(String newPasswordValue)

        (b) Revise API for all UI-related connector methods to include an IUIActivities object. IUIActivities object initially has just the convert/revert methods.

        (c) Revise connectors to use new method form, a connector at a time.

        Show
        Karl Wright added a comment - The strategy I will use to fix this is as follows: (a) Write a class that is referenced by the session bean, which mints password replacement values, and knows how to revert them. API: String convert(String passwordValue) String revert(String newPasswordValue) (b) Revise API for all UI-related connector methods to include an IUIActivities object. IUIActivities object initially has just the convert/revert methods. (c) Revise connectors to use new method form, a connector at a time.
        Hide
        Maciej Lizewski added a comment -

        or we could display obfuscated password in form elements. Then if posted value differs from stored one - it is assumed password changed and it is obfuscated and overwrites the one stored in config. It is still better to show obfuscated password in html source that clear text password

        Show
        Maciej Lizewski added a comment - or we could display obfuscated password in form elements. Then if posted value differs from stored one - it is assumed password changed and it is obfuscated and overwrites the one stored in config. It is still better to show obfuscated password in html source that clear text password
        Hide
        Karl Wright added a comment -

        Hi Maciej,

        Comparing against the "stored" value is what does not work, because there may well be no "stored" value. Obfuscation is reversible, but it's not very secure, and it is quite easy to come up with a password that can be deobfuscated into garbage. So I don't think that's going to work.

        Show
        Karl Wright added a comment - Hi Maciej, Comparing against the "stored" value is what does not work, because there may well be no "stored" value. Obfuscation is reversible, but it's not very secure, and it is quite easy to come up with a password that can be deobfuscated into garbage. So I don't think that's going to work.
        Hide
        Karl Wright added a comment -

        Good progress - I think I'll be done in time for 1.3.

        Show
        Karl Wright added a comment - Good progress - I think I'll be done in time for 1.3.
        Hide
        Karl Wright added a comment -

        r1503255 . Maciej, please check this out.

        Show
        Karl Wright added a comment - r1503255 . Maciej, please check this out.

          People

          • Assignee:
            Karl Wright
            Reporter:
            Maciej Lizewski
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development