Details

    • Type: Sub-task Sub-task
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: Release Branch 4.0, SVN trunk
    • Fix Version/s: None
    • Component/s: party
    • Labels:
      None

      Description

      Password are currently hashed but not seeded which may be a security issue.

        Issue Links

          Activity

          Hide
          Jacques Le Roux added a comment -

          Jeremy,

          Did you notice that they use a one way only encryption (ie no decryption is normaly possible, of couse even the better encryption algorithms known so far have been cracked)?

          If I remember well SHA-1 is used : http://en.wikipedia.org/wiki/SHA-1

          This may also interest you : http://www.nabble.com/How-do-I-decrypt-passwords--tf3081869.html#a8562707

          Show
          Jacques Le Roux added a comment - Jeremy, Did you notice that they use a one way only encryption (ie no decryption is normaly possible, of couse even the better encryption algorithms known so far have been cracked)? If I remember well SHA-1 is used : http://en.wikipedia.org/wiki/SHA-1 This may also interest you : http://www.nabble.com/How-do-I-decrypt-passwords--tf3081869.html#a8562707
          Hide
          Wickersheimer Jeremy added a comment -

          Seeding passwords is not related to the strength of the hashing algorithm, having the seed + hash is equally easy as having the hash because it is in the same place.

          It is critical to protect those hashes from dictionary attacks by attackers who obtained those hashes. ... like anyone using the webtool could do.

          Passwords should be protected as much as possible because it is very likely that a user password would be the same for other applications (Windows auth, emails, ...)

          Show
          Wickersheimer Jeremy added a comment - Seeding passwords is not related to the strength of the hashing algorithm, having the seed + hash is equally easy as having the hash because it is in the same place. It is critical to protect those hashes from dictionary attacks by attackers who obtained those hashes. ... like anyone using the webtool could do. Passwords should be protected as much as possible because it is very likely that a user password would be the same for other applications (Windows auth, emails, ...)
          Hide
          Guido Amarilla added a comment -

          Jeremy
          Are you talking about adding a salt to the password?

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

          In this case it would increase security, but the seed data passwords would become invalid.
          It would be even safer for each implementation if you keep this salt secret.

          Show
          Guido Amarilla added a comment - Jeremy Are you talking about adding a salt to the password? Ref: http://en.wikipedia.org/wiki/Salt_(cryptography ) In this case it would increase security, but the seed data passwords would become invalid. It would be even safer for each implementation if you keep this salt secret.
          Hide
          Wickersheimer Jeremy added a comment - - edited

          Yes,

          The nabble link is the problem exactly. Someone proposed to salt the passwords which is what should be done.

          The modification would be trivial really.

          • When you store a password you generate a random salt
          • Then you store in the DB two fields : the "salt" (hash of a randow string), and the "hashed(salt+password)"
            When you check a password, you just need to readd the salt before hashing and comparing to the DB.

          You can also concatenate the salt and hashed(salt+pass) in one field because both have predefined size.

          Show
          Wickersheimer Jeremy added a comment - - edited Yes, The nabble link is the problem exactly. Someone proposed to salt the passwords which is what should be done. The modification would be trivial really. When you store a password you generate a random salt Then you store in the DB two fields : the "salt" (hash of a randow string), and the "hashed(salt+password)" When you check a password, you just need to readd the salt before hashing and comparing to the DB. You can also concatenate the salt and hashed(salt+pass) in one field because both have predefined size.
          Hide
          Jacques Le Roux added a comment -

          I agree that salting could be a solution for dictionnary attacks. But why not replace the crypting algorithm by a newer and safer one (RIPEMD-160, SHA-256, Whirlpool, etc. ) my preference being SHA-256 ? It think it's easier, isn'it ?

          Show
          Jacques Le Roux added a comment - I agree that salting could be a solution for dictionnary attacks. But why not replace the crypting algorithm by a newer and safer one (RIPEMD-160, SHA-256, Whirlpool, etc. ) my preference being SHA-256 ? It think it's easier, isn'it ?
          Hide
          Jacques Le Roux added a comment -
          Show
          Jacques Le Roux added a comment - Just a link, not cheked : http://islab.oregonstate.edu/koc/ece575/03Project/Mundle/
          Hide
          Wickersheimer Jeremy added a comment -

          Replacing the algorithm won't change anything to the problem, and adding a salt is not difficult at all. If i have some time i will try to work on it.

          Show
          Wickersheimer Jeremy added a comment - Replacing the algorithm won't change anything to the problem, and adding a salt is not difficult at all. If i have some time i will try to work on it.
          Hide
          Jonathon Wong added a comment -

          > Replacing the algorithm won't change anything to the problem, and adding a
          > salt is not difficult at all. If i have some time i will try to work on it.

          I agree. SHA-256 is just as concrete a 1-way hash algo as MD5. Hashing a certain string will still always predictably produce a certain hash result, so hackers can easily work backwards to get the password.

          The common practice (for a very long time now) is to salt it. In fact, financial institutions even store the salt somewhere else, somewhere really safe.

          > It is critical to protect those hashes from dictionary attacks by attackers
          > who obtained those hashes. ... like anyone using the webtool could do.

          But wouldn't webtool also yield the salt as well?

          But still, I agree that salting the password would make dictionary attacks exponentially arduous. For a certain password, it is stored differently (thanks to random salt) in each instance where it is stored (say 2 or more users happen to like the same password). Therefore, for each hash stored in the database, an attack would have to do additional computation (dictionary attack has to be completely recoded, actually).

          For common needs, salting will adequately make dictionary attacks expensive (or impossible).

          It is incredibly easy to do dictionary attacks on un-salted password hashes.

          Show
          Jonathon Wong added a comment - > Replacing the algorithm won't change anything to the problem, and adding a > salt is not difficult at all. If i have some time i will try to work on it. I agree. SHA-256 is just as concrete a 1-way hash algo as MD5. Hashing a certain string will still always predictably produce a certain hash result, so hackers can easily work backwards to get the password. The common practice (for a very long time now) is to salt it. In fact, financial institutions even store the salt somewhere else, somewhere really safe. > It is critical to protect those hashes from dictionary attacks by attackers > who obtained those hashes. ... like anyone using the webtool could do. But wouldn't webtool also yield the salt as well? But still, I agree that salting the password would make dictionary attacks exponentially arduous. For a certain password, it is stored differently (thanks to random salt) in each instance where it is stored (say 2 or more users happen to like the same password). Therefore, for each hash stored in the database, an attack would have to do additional computation (dictionary attack has to be completely recoded, actually). For common needs, salting will adequately make dictionary attacks expensive (or impossible). It is incredibly easy to do dictionary attacks on un-salted password hashes.
          Hide
          Jonathon Wong added a comment -

          Guido,

          > In this case it would increase security, but the seed data passwords would
          > become invalid.

          The password hashes will certainly need to be recomputed if the new implementation adds a salt.

          In fact, to make brute force attacks even more expensive, the salt can be changed often (say every time the password hash is accessed) and the password hash recomputed. If it might take 24 hours for a super-computer to compute the password from a password hash and its salt, the salt could be changed every 23 hours.

          The ability to keep the salt secret in a strong and secure box will certainly be good. Not very cheap, though. Depends on how strong and how secure the salt box is.

          Show
          Jonathon Wong added a comment - Guido, > In this case it would increase security, but the seed data passwords would > become invalid. The password hashes will certainly need to be recomputed if the new implementation adds a salt. In fact, to make brute force attacks even more expensive, the salt can be changed often (say every time the password hash is accessed) and the password hash recomputed. If it might take 24 hours for a super-computer to compute the password from a password hash and its salt, the salt could be changed every 23 hours. The ability to keep the salt secret in a strong and secure box will certainly be good. Not very cheap, though. Depends on how strong and how secure the salt box is.
          Hide
          Michael Jensen added a comment -

          Is anyone working on this already?
          I'd like to help out with it. I'm kind of a hack w/ofbiz, so I'd need a little direction/mentoring on how to create an acceptable patch for the project.

          Show
          Michael Jensen added a comment - Is anyone working on this already? I'd like to help out with it. I'm kind of a hack w/ofbiz, so I'd need a little direction/mentoring on how to create an acceptable patch for the project.
          Hide
          BJ Freeman added a comment -

          couple of things to remember
          1) for creation of employees they like to use a password they will remember.
          2) when sending a new customer notification it should come back to a
          link that lets them change the password.
          3) products lets you define a default password. so add an option to have
          it auto created.
          there are a couple of places the hard coded passwords are.

          Show
          BJ Freeman added a comment - couple of things to remember 1) for creation of employees they like to use a password they will remember. 2) when sending a new customer notification it should come back to a link that lets them change the password. 3) products lets you define a default password. so add an option to have it auto created. there are a couple of places the hard coded passwords are.
          Hide
          Wickersheimer Jeremy added a comment -

          Hi,

          I am not working on it but i can help you if you need information.


          WICKERSHEIMER
          Jérémy

          Show
          Wickersheimer Jeremy added a comment - Hi, I am not working on it but i can help you if you need information. – WICKERSHEIMER Jérémy
          Hide
          Jonathon Wong added a comment -

          BJ said:
          > for creation of employees they like to use a password they will remember.

          For the sake of easy migration, we could add a temporary field beside each password, say "isRehashed" (boolean). If "isRehased" is false, process incoming password with the old codes without salt, and then rehash it with salt, and then set "isRehashed" to true.

          Show
          Jonathon Wong added a comment - BJ said: > for creation of employees they like to use a password they will remember. For the sake of easy migration, we could add a temporary field beside each password, say "isRehashed" (boolean). If "isRehased" is false, process incoming password with the old codes without salt, and then rehash it with salt, and then set "isRehashed" to true.
          Hide
          Michael Jensen added a comment -

          One option is to use the same field for the hash, but adding a colon and the salt to the end of the string. This is the way it would be easy to distinguished between salted and non-salted passwords and validate accordingly. I've seen a few projects that store password hashes this way.
          The Linux /etc/shadow file also stores the hash and salt in one field (but that doesn't mean it is best for this situation.)

          An alternative could be to just have the salt stored in another field in the same table and if it isn't empty, the password hash is salted. (You have to store the salt somewhere anyway.)

          Show
          Michael Jensen added a comment - One option is to use the same field for the hash, but adding a colon and the salt to the end of the string. This is the way it would be easy to distinguished between salted and non-salted passwords and validate accordingly. I've seen a few projects that store password hashes this way. The Linux /etc/shadow file also stores the hash and salt in one field (but that doesn't mean it is best for this situation.) An alternative could be to just have the salt stored in another field in the same table and if it isn't empty, the password hash is salted. (You have to store the salt somewhere anyway.)
          Hide
          Jonathon Wong added a comment -

          Oh yes! That will do away with the need for a new entity field like "isRehashed". I would like the salt to be appended to the password hash after a ":", so we don't need to create a new entity field for the salt. The ':' character doesn't appear in a password hash.

          Show
          Jonathon Wong added a comment - Oh yes! That will do away with the need for a new entity field like "isRehashed". I would like the salt to be appended to the password hash after a ":", so we don't need to create a new entity field for the salt. The ':' character doesn't appear in a password hash.
          Hide
          Jacques Le Roux added a comment -

          I will create soon a general task for security issues. All current pending security issues will be children of this new task.

          Show
          Jacques Le Roux added a comment - I will create soon a general task for security issues. All current pending security issues will be children of this new task.
          Hide
          Marco Risaliti added a comment -

          Sorry Jacques, I have not seen that it was a grouped bugs.
          In this case I have used to set in the grouped bugs the sum of the components used by detailed issues.
          I didn't like unknow components.
          Otherwise we can add a new fictitious component - GROUPED ISSUES - and assign this component to this type of issue.

          Thanks
          Marco

          Show
          Marco Risaliti added a comment - Sorry Jacques, I have not seen that it was a grouped bugs. In this case I have used to set in the grouped bugs the sum of the components used by detailed issues. I didn't like unknow components. Otherwise we can add a new fictitious component - GROUPED ISSUES - and assign this component to this type of issue. Thanks Marco
          Hide
          Jacques Le Roux added a comment -

          Marco,

          Do you know how to create a new component in Jira ? I never tried

          Show
          Jacques Le Roux added a comment - Marco, Do you know how to create a new component in Jira ? I never tried
          Hide
          Marco Risaliti added a comment -

          Hi Jacques,

          I have not the grant to the administration of JIRA and so I cannot help you on how to create a new component.

          Thanks
          Marco

          Show
          Marco Risaliti added a comment - Hi Jacques, I have not the grant to the administration of JIRA and so I cannot help you on how to create a new component. Thanks Marco
          Hide
          Jacques Le Roux added a comment -

          Marco,

          Done, was a good idea, at least I hope everybody will think so.

          Show
          Jacques Le Roux added a comment - Marco, Done, was a good idea, at least I hope everybody will think so.
          Hide
          Marco Risaliti added a comment -

          Also I like this workaround to see how many INCORPORATING ISSUE are active.
          Before switch the others INCORPORATING ISSUE to this new fictitious components I will wait some other feedback from others.

          Thanks
          Marco

          Show
          Marco Risaliti added a comment - Also I like this workaround to see how many INCORPORATING ISSUE are active. Before switch the others INCORPORATING ISSUE to this new fictitious components I will wait some other feedback from others. Thanks Marco
          Hide
          Jacques Le Roux added a comment -

          Maybe we should just provide a salting mechanism with clear explanations. I mean OFBiz paswords salted OOTB but only as a demonstration and clear explanations about not only changing passwords (as it's already done for admin password) but also salt string. Maybe Michael Jensen's idea of colon separating password and salt could be used ? I also remember the idea of having a salt string only related to the password at hand (to avoid easy hack if the salt is discovered by a way or another...), this is also called random salt (the alternative being static salt). But obviously this introduces a new breach has you have to store also the random salt. Except if you use a part of the record only *you*know (for instance a part of the creation date field, etc.)

          My 2cts

          Jacques

          Show
          Jacques Le Roux added a comment - Maybe we should just provide a salting mechanism with clear explanations. I mean OFBiz paswords salted OOTB but only as a demonstration and clear explanations about not only changing passwords (as it's already done for admin password) but also salt string. Maybe Michael Jensen's idea of colon separating password and salt could be used ? I also remember the idea of having a salt string only related to the password at hand (to avoid easy hack if the salt is discovered by a way or another...), this is also called random salt (the alternative being static salt). But obviously this introduces a new breach has you have to store also the random salt. Except if you use a part of the record only *you*know (for instance a part of the creation date field, etc.) My 2cts Jacques
          Hide
          Adam Heath added a comment -

          I actually have a patch for this now. Existing database entries will continue to work, while changing a password will end up being salted. The salt is randomly generated each and every time a password is hashed. There is no globally shared salt at any point. The length of the salt is from 1 to 16 chars, and the content is also random.

          Show
          Adam Heath added a comment - I actually have a patch for this now. Existing database entries will continue to work, while changing a password will end up being salted. The salt is randomly generated each and every time a password is hashed. There is no globally shared salt at any point. The length of the salt is from 1 to 16 chars, and the content is also random.
          Hide
          Martin Kreidenweis added a comment -

          Adam,

          great news about the patch. Where can I get it?

          Thanks
          Martin

          Show
          Martin Kreidenweis added a comment - Adam, great news about the patch. Where can I get it? Thanks Martin
          Hide
          Adam Heath added a comment -

          I've just committed a change that finally implements this(svn 1327741). There are a few remaining buglets, however, in the existing seed/demo data that I'm working thru.

          Show
          Adam Heath added a comment - I've just committed a change that finally implements this(svn 1327741). There are a few remaining buglets, however, in the existing seed/demo data that I'm working thru.
          Hide
          Jacques Le Roux added a comment -

          Hi Adam,

          Would you need some help to fix the seed/demo data?

          Show
          Jacques Le Roux added a comment - Hi Adam, Would you need some help to fix the seed/demo data?
          Hide
          Adam Heath added a comment -

          Technically, any hard-coded value, even hashed, in the seed data is bad. It'd be nice to get different per-install salt+hash values in the database. However, the only way to do that would be to store the non-hashed passwords in seed, and salt+hash them during store. That would require a change to the xml data loader.

          I haven't done any of this, am just brainstorming.

          If we do not go this route, then each stored hashed value should be changed to a different salt+hash value. There is a simple main(String[]) command in the repo that can facilitate this.

          Show
          Adam Heath added a comment - Technically, any hard-coded value, even hashed, in the seed data is bad. It'd be nice to get different per-install salt+hash values in the database. However, the only way to do that would be to store the non-hashed passwords in seed, and salt+hash them during store. That would require a change to the xml data loader. I haven't done any of this, am just brainstorming. If we do not go this route, then each stored hashed value should be changed to a different salt+hash value. There is a simple main(String[]) command in the repo that can facilitate this.
          Hide
          Jacques Le Roux added a comment -

          I see, thanks for comment Adam

          Show
          Jacques Le Roux added a comment - I see, thanks for comment Adam

            People

            • Assignee:
              Adam Heath
              Reporter:
              Wickersheimer Jeremy
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development