Skip to content
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

DKIM verification fails due to msg body modification by private relays/transfer agents #165

Open
winnsterx opened this issue Jan 31, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@winnsterx
Copy link

An edge case that causes DKIM verification failure is when the email comes from a private relay service or a transfer agent. These intermediaries modify the body without updating the expected hash in the header.

One specific example is if Apple users select to "sign-in with Apple ID" for certain apps (i.e. Yelp), Apple will relay the email from Yelp to the receiver all under emails hosted by privaterelay.appleid.com. Even though the signature passes, the body hash check fails.

    const dkimResult = await dkimVerify(rawEmail);
    console.log(dkimResult)

spits out the following error:

    {
      headerFrom: [
        'no-reply_at_mail_yelp_com_xdv2qypf6k_d3e906cd@privaterelay.appleid.com'
      ],
      envelopeFrom: '[email protected]',
      results: [
        {
          signingDomain: 'privaterelay.appleid.com',
          selector: 'prv2019',
          // abbreviated
          bodyHash: 'y/pRNiQ3oAGue/rXnUlbK8RE5WJKj5yqLqfVTXbD9vQ=',
          bodyHashExpecting: 'hf7kLUsqHDsk69NP7DoTs/lTLUh4F88Ec0c3kQ42a3k=',
          // abbreviated
          info: 'dkim=neutral (body hash did not verify) [email protected] header.s=prv2019 header.a=rsa-sha256 header.b=XjoKCxjS'
        }
      ]
    }

One way to fix this is to skip the body hash check entirely if we detect that the email is coming from a whitelisted relay service. Or one can check exactly where Apple is tempering with the body and excise that section to get the body hash.

@winnsterx winnsterx changed the title dkimVerify fails due to message body modification by private relays or MTAs DKIM verification fails due to msg body modification by private relays Jan 31, 2024
@winnsterx winnsterx changed the title DKIM verification fails due to msg body modification by private relays DKIM verification fails due to msg body modification by private relays/transfer agents Jan 31, 2024
@Divide-By-0
Copy link
Member

Divide-By-0 commented Feb 4, 2024

@VasuK111 this will also be helpful for you to fix this edge case!

We should aim to undo their changes and verify the original dkim anyways.

Might also be good to include the modifications in #162 here as well. It's because ideally, to avoid people falling back to ARC when DKIM could work, I think best for possible DKIM verifications to be tried before ARC is tried. The zkp2p fixes are here:

see (specific to HDFC): https://github.com/zkp2p/zk-p2p/blob/develop/client/src/components/ProofGen/validation/hdfc.tsx#L85
and: https://github.com/zkp2p/zk-p2p/blob/develop/client/src/components/ProofGen/validation/venmo.tsx#L62

@Divide-By-0 Divide-By-0 added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Feb 4, 2024
@Divide-By-0 Divide-By-0 mentioned this issue Feb 18, 2024
9 tasks
@star-gazer111
Copy link

star-gazer111 commented Feb 18, 2024

But how do I whitelist beforehand a particular relay service? @Divide-By-0 @winnsterx . Any thoughts please?

Edit from Divide-By-0: We are not whitelisting relays, we are reverting the email.

@saleel
Copy link
Member

saleel commented Apr 3, 2024

#189 might fix this
If not, a specific sanitizer can be added here if we know what is the change made by apple -

const sanitizers = [revertGoogleMessageId, removeLabels, insert13Before10];

On another note, if there is not requirement to extract data from email body, then bodyhash check can be ignored altogether on the circuit level. Both circuit and input generation helpers have parameter for ignoreBodyHashCheck.

@Divide-By-0
Copy link
Member

@saleel was this fully fixed with tests? i forget

@saleel
Copy link
Member

saleel commented May 29, 2024

We do have the infra and some cases are handled, but not sure if it covers the Apple case mentioned above.

If its not covered, we need to identity the changes made by Apple and add a sanitizer to revert that.

@winnsterx In you email do you also have a dkim signature from yelp? If so, its better to use with that, so verifier is trusting yelp signature and not apple-relayer (which could have same key for other domains as well?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants