Open-Attestation / oa-encryption

MIT License
1 stars 0 forks source link

Symmetrical key is likely being revealed over the public wire #10

Open kdenhartog opened 3 years ago

kdenhartog commented 3 years ago

Based on what I can tell about the design of IEncryptionResult, if this object is being passed over the wire (e.g. directly encoding into the QR code) then the confidentiality and integrity guarantees of the ciphertext are removed because an active adversary can intercept, decrypt, and regenerate the payload in an unexpected way.

Rather than directly embedding the symmetrical key within the EncryptionResult the symmetrical key should be generated using something like a key agreement scheme to have the symmetrical key be generated between the sender and recipient of the message.

rjchow commented 3 years ago

Thanks for the feedback!

I was thinking that's a transport level concern, as it should already use TLS or some other form of SSL? The decryption key really is just a means of authenticating the request rather than providing confidentiality

kdenhartog commented 3 years ago

Gotcha, if the only goal is integrity guarantees I'd suggest looking at digital signatures instead. While AES-GCM does integrate a MAC in with it, the encryption is a bit overkill if you're not looking to get confidentiality from this layer and because you're passing the symmetrical key within the object you actually end up breaking your integrity guarantees at this layer too.

Given it looks like the intended payload is meant to be an open attestation and those are already signed, I think you could actually just remove this layer and rely on TLS for confidentiality and then rely on the signature of the attestation for authenticity.

Unfortunately, I'm not completely aware of your entire architecture to say with certainty, but if you point me to some documentation I'd be happy to review it and provide better advice on if this is needed.

rjchow commented 3 years ago

Sorry - I meant to reply this this earlier

We've got a document here with a proposal that's been implemented https://github.com/Open-Attestation/adr/blob/master/universal_actions.md

So the point of this encryption library is really so that we can expose encrypted blobs publicly and still have access control via decryption key instead of file access permissions - TLS won't help with that

pgaulon commented 3 years ago

Hello @rjchow !

I second @kdenhartog : the fact that the AES-GCM symmetric key is used during the verify as a GET parameter (along the URL to retrieve the encrypted data), makes it accessible to whoever is allowed to read the web server logs, browser history, QR history. This is actually a common vulnerability from OWASP.

From what I understand, this is deployed through a third party CDN, and read by anyone with a QR reader, so they will have access to all the data contained in the encrypted blob, as it is trivial to decrypt. Since the data is mainly PII and health related, it could be an important issue.

rjchow commented 3 years ago

Hello @pgaulon - thanks for raising this! I don’t think it’s been documented yet but we’ve moved the decryption key to the anchor/fragment part of the url which isnt usually sent in the HTTP request for most well behaved clients.

i think the part about it being in the QR is not trivially solvable without greatly increasing user friction - do you have any suggestions on that?

pgaulon commented 3 years ago

Ah indeed you are right @rjchow , the verification can use the anchor key. And it is actually documented. However newly (as of today) submitted confirmation emails and QR still contain the old format that sends the key as GET. The key and payload URL are also sent to other websites like the assets/analytics ones (Wogaa and Adobe), as the scripts loaded from there take the full URL to load additional scripts. The anchor is actually being sent by clients to those ones.

If I were to suggest something without much knowledge, I would say that the QR containing the key is not much of a problem in itself (the customer needs to store that symmetric key somehow). To make sure that user agents do not send that key, the QR format could be changed to something that cannot be directly consumed by classic QR readers. A base64 of the json blob for instance. And a verification client mobile application could be created, to decode the base64, consume the different elements of the QR code to verify its content only in memory (from what I understand verify the GCM tag, get the sha3 of the decrypted text and get the signer with the TXT record to match it). But as you rightfully said, at the UX cost of having one more application to read the QRs.

Nebulis commented 3 years ago

@pgaulon i thought notarise was updated. I will check with the team. Thanks for your feedback

HJunyuan commented 3 years ago

Notarise has now been updated to generate URLs with anchor key.