eu-digital-identity-wallet / eudi-lib-jvm-sdjwt-kt

A library for issuing and verifying SD-JWT
Apache License 2.0
14 stars 4 forks source link

Problems with decoy generation mechanism #199

Closed markuskreusch closed 3 weeks ago

markuskreusch commented 1 month ago

The current approach for decoy generation works by configuring a maximum number of decoys globally for an sd-jwt generation and then generating a random number of decoys for each sd-claim between 1 and this maximum.

This approach does not necessarily hide the information about claim number as desired. Lets assume we have a simple membership credential. Lets assume the maximum number of decoys is set to 5.

Claims if user is a premium member:

{
  "name": "Markus",
  "premiumMembershipNumber": 1234
}

Claims if user is not a premium member:

{
  "name": "Markus"
}

In this case the number of claims (1 or 2) discloses the information if the holder is a premium member. We assume that the membership number is made selectively disclosable. Thus the resulting digest number for premium members will be 3 - 7 digests while 2 - 6 for non members. The presence of 2 or 7 digests discloses information about the membership.

Any digest number from 3 - 6 does not directly disclose information. It may happen though, that over time the holder sends several SD-JWTs all with the same data but different number of digests to a verifier. This can happen for privacy reasons to prevent correlation or over time after the SD-JWTs expire and are reissued. The verifier could then compute the average number of digests which will be 4 for non members and 5 for members.

A suitable mechanism to prevent this leakage of information is needed. The following should work: On each level of the claims the maximum number of claims must be known. The implementation should always add as many decoy digests that this maximum is reached. This way the number of digests visible to a third party will be always the same and no information is revealed.

In addition the decoy number generation uses Random instead of SecureRandom which imposes an additional problem: if (numOfDecoysLimit >= 1) Random.nextInt(1, numOfDecoysLimit) else 0 Also if I read above code correctly the invocation will fail if numOfDecoysLimit is 1 because the end index in Random.nextInt is exclusive and validated. This is an assumption from the docs, I did not run the code.

babisRoutis commented 1 month ago

Dear @markuskreusch

Thanks for reporting this. For sure, some time will be needed to process this :smile:

Until then, please allow me a couple initial comments:

The examples that you provided don't reflect the exact behavior of the library. The library adds 1..numOfDecoysLimit decoys for every selectively disclosable element. So using your example claims the number of disclosures would be

The main point of the issue raised, I have the impression that assumes that the issuer will have a single instance of SdJwtIssuer with a pre-configured number for numOfDecoysLimit. This doesn't have to be the case though. An issuer may decide to instantiate the issuer with another policy (for instance, use different numOfDecoysLimit for issuing credentials to the same holder, or arbitrary choose a number for every issuance).

babisRoutis commented 1 month ago

Dear @markuskreusch

I would appreciate if you could check the #203

The PR improves DSL and allows you to explicitly specify (at every level) the desired number of digests

fun sdJwtSpec(membership: Membership) = sdJwt(desiredDigests = 5) {
        sd("name", membership.name)
        if (membership is Membership.Premium) {
            sd("premiumMembershipNumber", membership.premiumMembershipNumber)
        }
    }

And you will get 5 digests in simple & premium case If you are interested just check DecoyTest and provide comment directly to the PR

babisRoutis commented 1 month ago

In addition the decoy number generation uses Random instead of SecureRandom which imposes an additional problem:

FYI created #206

markuskreusch commented 3 weeks ago

Hello @babisRoutis

Sorry for not having the time to review the PR before the release but looks all good to me. Thank you for the quick fix of this issuer :)

babisRoutis commented 3 weeks ago

Hello @babisRoutis

Sorry for not having the time to review the PR before the release but looks all good to me. Thank you for the quick fix of this issuer :)

No worries @markuskreusch

I think that it works better now, you were right. Also, as a bonus, support for decoys was added for arrays (simple or recursively disclosed).

Thank you