-
Notifications
You must be signed in to change notification settings - Fork 25
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
include verifier's public encryption key into SessionTranscript to prevent some possible attacks in case of unsigned requests #400
Comments
Just noting that if the DCP working group believes this is needed, then we would need to do this for all credential formats. |
I would like to understand the attack scenario clearly and have assurance that there is significant benefit of this change, before going down this route |
This. So much this. If there's a there there, then we need to address it for the entirety of OpenID4VP. If there's no there there, can we please limit the distractions on participants already limited time and energy. [Edit: "there is no there there" is an idiom that might not be as well known as I imagined when writing the above] |
The reason for asking the question earlier was because I was wondering what the reason was behind not including the RP public key from the session transcript and whether it was related to a downside for including it or not being aware of any benefits for including it. |
I understand the attack, but I’m wondering whether this measure can actually prevent any damage. The wallet wouldn't necessarily detect it, only the RP may. An attacker would still learn the information provided by the wallet regardless, right? |
|
I think a specific attack would need to be presented here to justify adding this. I think the scenario being presented here is that an attacker is able to make unsigned requests from within the origin context of a legitimate RP, e.g they can run javascript in the context of a legitimate RP's website. If you think of two different attacks:
For this to be useful, I think there needs to be an attack where the RP gets the response and is able to decrypt it successfully to get the credential. But then when checking the session transcript within that credential, detects the key was modified. I'm not sure this is a situation that could ever happen. Regarding the point that "it doesn't really hurt to add something to the transcript, even if the value is marginal". I think I do disagree with this. There is overhead in figuring out this for every format and developer overhead in having to deal with it. So I do think we should hold somewhat of a high bar for things that need to signed over by credential and independently reconstructed by the caller. We'd need to document a specific attack that would justify making a change that can be supported by all credential formats |
I concur with the sentiment @leecam expresses here. |
I think the second part could be different: the attack decrypts the response, could choose to re-encrypt with the correct encryption key for the RP, and forward to the RP. That way an attacker (e.g., compromised or malicious browser) could read responses of unsigned but enrypted flows and there would be no real way to detect it:
My questions would be:
|
Yeah I was thinking about this attack last night too and I think its totally valid. Then was trying to figure out if we care about it :) I think for the case where the attacker is in a position to modify the request and response in the context of the origin, the attacker is still going to get the user data. So its a question of its worth trying to make it detectable. Is it only detectable by the user? In that the attacker could return USER_CANCELLED to the RP. So the RP doesn't see anything suspicious. But the user will see it fail, even through they presented their credential correctly. Anyhow thinking about it last night I do think you could make a case to binding the verifier key in the session transcript for this specific attack. I don't think it really prevents an attack, or stops the attacker getting the data. But it could make an attack slighter harder to pull off, as either the user or the RP would detect it. |
On today's ISO mDL WG call, Martijn asked if there was a reason why the verifier's public encryption key isn't included in the SessionTranscript.
Including it there might prevent some 'man in the browser' type attacks (or any case where a component between relying party and wallet changes the encryption key) for unsigned requests where the encryption key has been replaced by the attacker. (Martijn I believe also noted that in these kind of scenarios the attacker has already gained significant access, so preventing this exact attack may not be worthwhile. But also mentioned that including the key in the SessionTranscript might not be harmful so maybe it should be done anyway.)
The text was updated successfully, but these errors were encountered: