Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: Trunk
    • Component/s: framework
    • Labels:
      None
    • Sprint:
      Bug Crush Event - 21/2/2015

      Description

      It looks as though no salt data is used when saving encrypted entity data making the stored data susceptible to dictionary attacks.

      If you look through the stored demo data, you can see all the demo accounts passwords are the same:

      UserLogin:
      admin     {SHA}47ca69ebb4bdc9ae0adec130880165d2cc05db1a
      flexadmin {SHA}47ca69ebb4bdc9ae0adec130880165d2cc05db1a
      ...
      

      As a comparison, if you create a two unix accounts, "ofbiz1" and "ofbiz2" and set both passwords to "ofbiz"

      ofbiz1:$6$3.mYZg9u$0E...:14524:0:99999:7:::
      ofbiz2:$6$MJhYeMqO$Jf...:14524:0:99999:7:::
      

      You can see that on unix, even though the passwords are the same, the encrypted values are completely different.

      For more information see:

      http://en.wikipedia.org/wiki/Salt_(cryptography)

        Issue Links

          Activity

          chris snow created issue -
          Jacques Le Roux made changes -
          Field Original Value New Value
          Link This issue is part of OFBIZ-1151 [ OFBIZ-1151 ]
          Jacques Le Roux made changes -
          Link This issue is part of OFBIZ-1151 [ OFBIZ-1151 ]
          Jacques Le Roux made changes -
          Parent OFBIZ-1525 [ 12384719 ]
          Issue Type Bug [ 1 ] Sub-task [ 7 ]
          Jacques Le Roux made changes -
          Link This issue blocks OFBIZ-1151 [ OFBIZ-1151 ]
          Gavin made changes -
          Workflow jira [ 12478819 ] OFbiz Workflow [ 12504183 ]
          Adam Heath made changes -
          Assignee Adam Heath [ doogie ]
          Hide
          Adam Heath added a comment -

          I'm working on this today. Should have it implemented by this evening. Won't be able to commit it right away, as I need to split the commit up into smaller chunks. The changes are in EntityCrypto.

          Show
          Adam Heath added a comment - I'm working on this today. Should have it implemented by this evening. Won't be able to commit it right away, as I need to split the commit up into smaller chunks. The changes are in EntityCrypto.
          Jacopo Cappellato made changes -
          Component/s framework [ 12311145 ]
          Hide
          Adam Heath added a comment -

          This issue is about using salt for one-way hashes. That particular feature has been implemented for 2 years. I'll be verifying the demo data, to see how much of the hashed passwords are the same, and changing them if that is the case.

          As for using salt for bi-directional encryption, that's more problematic. If you want to be able to do exact-match lookups on encrypted values, then you can't use salt, as the stored value, and the encrypted value in the database WHERE clause, would end up being different(due to random salt bytes being pre-pended). See OFBIZ-5659 for a more detailed discussion.

          Show
          Adam Heath added a comment - This issue is about using salt for one-way hashes. That particular feature has been implemented for 2 years. I'll be verifying the demo data, to see how much of the hashed passwords are the same, and changing them if that is the case. As for using salt for bi-directional encryption, that's more problematic. If you want to be able to do exact-match lookups on encrypted values, then you can't use salt, as the stored value, and the encrypted value in the database WHERE clause, would end up being different(due to random salt bytes being pre-pended). See OFBIZ-5659 for a more detailed discussion.
          Hide
          Jacques Le Roux added a comment -

          Your last comment makes now (that I'm refreshed) totally sense for me Adam. So indeed salt should only be used with SHA1 not MD5 or other bi-directional encryptions.

          Show
          Jacques Le Roux added a comment - Your last comment makes now (that I'm refreshed) totally sense for me Adam. So indeed salt should only be used with SHA1 not MD5 or other bi-directional encryptions.
          Hide
          Adam Heath added a comment -

          Jacques, SHA1 and MD5 are the same kind of technology; they are both one-way hashes. Either could be used for password-type fields. Originally, it was MD5. I added an ability to have different hashes(I stored the hash type in the database), then added salt ability(as a prefix).

          The examples given are for password-type fields. As that support has been around for quite a while now, this issue can be closed.

          Additionally, if you really are concerned about encrypted fields not having a salt, then svn trunk now supports such a feature. In the entitymodel definition, you can set encrypt="salt", and the value saved to the database will have a salt pre-pended. Note, that if you do this, then you will not be able to do direct lookups against that value.

          Show
          Adam Heath added a comment - Jacques, SHA1 and MD5 are the same kind of technology; they are both one-way hashes. Either could be used for password-type fields. Originally, it was MD5. I added an ability to have different hashes(I stored the hash type in the database), then added salt ability(as a prefix). The examples given are for password-type fields. As that support has been around for quite a while now, this issue can be closed. Additionally, if you really are concerned about encrypted fields not having a salt, then svn trunk now supports such a feature. In the entitymodel definition, you can set encrypt="salt", and the value saved to the database will have a salt pre-pended. Note, that if you do this, then you will not be able to do direct lookups against that value.
          Adam Heath made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Fix Version/s SVN trunk [ 12311928 ]
          Resolution Fixed [ 1 ]
          Hide
          Jacques Le Roux added a comment -

          Yes confused with MD5 indeed, thanks for the info about encrypt="salt"

          Show
          Jacques Le Roux added a comment - Yes confused with MD5 indeed, thanks for the info about encrypt="salt"
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Closed Closed
          1720d 18h 1 Adam Heath 24/Jun/14 02:36

            People

            • Assignee:
              Adam Heath
              Reporter:
              chris snow
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile