search

trusteddomainproject / OpenARC

Open source ARC implementation
BSD 2-Clause "Simplified" License
135 stars 45 forks source link

Fix invalid ARC-Seal when email contains existing sets #167

Open abeverley opened 9 months ago

abeverley commented 9 months ago

When creating the ARC-Seal, any existing sets in the email must be included in the seal. See https://www.rfc-editor.org/rfc/rfc8617#section-5.1.1

The existing code does not include existing sets and is thus creating invalid signatures in these circumstances. This PR includes them.

flowerysong commented 9 months ago

I'm not sure this is correct, though the code structure's fairly complicated and I don't really have time right now to try to grok it fully.

This should already have been done by arc_canon_runheaders_seal(), I believe. I can certainly verify that my non-milter use of libopenarc has not had a problem generating valid seals when an email contains existing sets, so it feels like it's probably something more subtle than them never being included.

abeverley commented 9 months ago

Thanks @flowerysong I see what you mean. If I get a chance I will have another look.

abeverley commented 9 months ago

Yes - the problem is at this line: https://github.com/trusteddomainproject/OpenARC/blob/eb430dbdeee9f502295fe7a7d5041dfca3f00745/libopenarc/arc.c#L2915

It only runs arc_canon_runheaders_seal() when verifying, not signing. Making it run during signing too fixes the problem.

I assume it's as simple as removing the condition for (msg->arc_mode & ARC_MODE_VERIFY) != 0, rather than adding a second condition for (msg->arc_mode & ARC_MODE_SIGN) != 0? (Which would presumably always be true?)

flowerysong commented 9 months ago

Arguably that's a configuration error, and if you're signing messages that might already have ARC chains you should be setting the mode to ARC_MODE_VERIFY | ARC_MODE_SIGN.

I don't really feel like making that argument, though. Having a trap configuration that's maybe slightly more performant when you're purely a message originator but breaks for everyone else doesn't seem like a good idea.

abeverley commented 9 months ago

Arguably that's a configuration error, and if you're signing messages that might already have ARC chains you should be setting the mode to ARC_MODE_VERIFY | ARC_MODE_SIGN.

I've actually got no mode defined at all in the configuration, so it's just the default value. From the manual: "If neither is specified, the operating mode will be inferred on a per-connection basis based on the entries in the InternalHosts list; connections from internal hosts will be assigned to signing mode, and all others will be assigned to verify mode".

So I guess it's automatically setting the mode to "sign", which then means arc_canon_runheaders_seal() is not executed.

I don't really feel like making that argument, though. Having a trap configuration that's maybe slightly more performant when you're purely a message originator but breaks for everyone else doesn't seem like a good idea.

Agreed - as it is the mode needs to include "verify" in order to properly "sign" ;-)

I'll take a look to see if there is any way to still execute conditionally, but it would seem to make sense to always run that code as it could be needed for verification or signing.

Thanks for your comments.

abeverley commented 9 months ago

Based on the above discussion, I have changed this PR to always verify previous ARC sets, regardless of the mode.