-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add key change implementation #2
Comments
Do I need to do anything for this? |
I don't think so. You said your merge-mrochlin branch is ready for merging into master, right? |
I think so. Michael Rochlin On Wed, Mar 16, 2016 at 9:09 PM, Marcela [email protected] wrote:
|
@marisbest2 Could you please clarify: Why does your implementation change the change key after each ULN change? If I'm reading it correctly, the change key change is never itself authenticated (even in the signed key change case). I couldn't find an explanation in your report. |
I think that you're referring to KeyChange.java. Assuming that's the case, If that's not what you're referring to, let me know. Michael Rochlin On Thu, May 19, 2016 at 1:43 PM, Marcela Melara [email protected]
|
I did see that, though that isn't what I was referring to (I suppose it's related). I was referring to your code in the test client, specifically why you generate a new key pair for the change key every time you change the blob or the key change policy. This new change key pair is never authenticated, so even if the user doesn't allow unsigned key/ULN changes, the server can still insert a spurious blob by replacing the public change key in the ULN. |
Updating the changekey by the client every time was just a proof of concept If the user requires only signed key changes, then I think the server would I may be overlooking something, but I think that's right. Michael Rochlin On Thu, May 19, 2016 at 3:18 PM, Marcela Melara [email protected]
|
I see. I'm not sure what a malicious server could gain from replaying an old key-change-message+signature, but I might be missing something there. I think you're right, if a user requires signed key changes and if the server tries to update the user's blob using a different previous change key, the signature verification on the change of key message should fail. But I'm concerned with the fact that not signing the new change key opens a window for a malicious server to equivocate about the change key, which then allows it to insert spurious blobs in the future. So for example, if the Alice's change key was ck1, her new blob is blob2 and the new change key is ck2, the malicious server can equivocate presenting ck2' + sign_ck1(blob2) to Bob, and then subsequently send Bob ck3 + sign_ck2'(blob3). Whereas, if Alice's client includes the change key: ck2 + sign_ck1(blob2+ck2), Bob's client can immediately reject ck2' + sign_ck1(blob2+ck2) because the change keys don't match and ck2' wasn't authorized by Alice's client. It's true that Bob's client will eventually detect the equivocation even if the new change key isn't signed by Alice's client since the change key is included in the ULN. So maybe not signing the new change key doesn't make things worse since we already allow unsigned blob changes? But I do think that signing the new change key protects users who require signed key changes from such an equivocation attack. |
I agree on the replay attack, and I think we had discussed it and said As for the signature, i think the entire message, including the new change On Tuesday, May 24, 2016, Marcela Melara [email protected] wrote:
Michael Rochlin |
Or at least it should be. If it's not then that's a bug, not a design On Tuesday, May 24, 2016, Michael Rochlin [email protected] wrote:
Michael Rochlin |
You're absolutely right, the client does sign the entire message including the change key. I was confused on what the message it was signing was -- I didn't realize the ULNChangeReq contains the entire ULN contents. Thanks for clarifying, and so sorry for not paying more attention from the start. |
Its ok. It took me a while to figure that out, and it should probably be Michael Rochlin On Tue, May 24, 2016 at 10:58 PM, Marcela Melara [email protected]
|
This requires merging @marisbest2's code in the internal CONIKS repo to this repo.
The text was updated successfully, but these errors were encountered: