EDIT: OK, I think I see what's wrong with this analysis: https://github.com/18F/identity-idp/blob/master/spec/service....
They do include Z1 in the data they pass to KMS decrypt. This is because they have XORed the ciphertext with Z1 - so now in order to recover the initial ciphertext back (i.e. the cipher text you can decrypt with KMS to get R back), you need to XOR with Z1 again.
So even if you get D from the database, you can't decrypt it unless you have Z1, and if you were trying to brute-force it, you would need to call KMS for every attempt with the Z1 for this attempt.
Original comment:
I am not any kind of crypto expert. But I agree with you.
I think they should be XORing with "AssignedSecret" (not "EncryptedAssignedSecret"). Then in order to compare password hashes you would need to decrypt the "EncryptedAssignedSecret" using KMS. (using the terminology in the description of the algorithm here: https://github.com/18F/identity-idp/blob/master/docs/encrypt...) (EDIT: you actually cover this approach in the GH issue, and point out a problem with it).
I think there is another approach like what the OP proposed where they should be using some portion of the scrypt hash (either Z1 or Z2 or just the whole thing) as part of the encryption context passed to KMS. But this seems a little more complicated to me. (EDIT: You also mention this as a possible solution in the GH issue).
Where you get the key and the key's cipher text back from KMS and store that, and then you have to use KMS to decrypt whatever you encrypted with the key.
Yes, you need to know the password to decrypt. That's the basis of their very interesting PII protection scheme!
My point is you can brute-force the password without the HSM. And their docs specifically say this should not be possible. Isn't the whole point of the HSM to prevent that?
'hash' is derived from 'z2' and 'd' - and they give you 'd'. Password will produce 'z2'. So that's the brute-force attack.
To say it another way, the 'z1' path is secured. But there is the 'z2' path and it's wide open.
I've left a comment on your github issue, but the tl;dr is that the encryption document doesn't seem to reflect the code. The comment in the ruby test case suggests that the actual step is 'E = hash( Z2 + R )' instead of 'E = hash( Z2 + D )' where 'R' is the user's randomly generated 'AssignedKey' and 'D' is the masked HSM ciphertext 'KMS(R) ^ Z2'.
Perhaps somewhere during implementation someone realized the document doesn't make sense as written.
Original comment:
I am not any kind of crypto expert. But I agree with you.
I think they should be XORing with "AssignedSecret" (not "EncryptedAssignedSecret"). Then in order to compare password hashes you would need to decrypt the "EncryptedAssignedSecret" using KMS. (using the terminology in the description of the algorithm here: https://github.com/18F/identity-idp/blob/master/docs/encrypt...) (EDIT: you actually cover this approach in the GH issue, and point out a problem with it).
I think there is another approach like what the OP proposed where they should be using some portion of the scrypt hash (either Z1 or Z2 or just the whole thing) as part of the encryption context passed to KMS. But this seems a little more complicated to me. (EDIT: You also mention this as a possible solution in the GH issue).
I think there's also an approach using the GenerateDataKey API : http://docs.aws.amazon.com/kms/latest/APIReference/API_Gener...
Where you get the key and the key's cipher text back from KMS and store that, and then you have to use KMS to decrypt whatever you encrypted with the key.