search

lieser / dkim_verifier

DKIM Verifier Extension for Mozilla Thunderbird
MIT License
210 stars 35 forks source link

Update the ARH parser to be compliant with RFC 7601 #52

Closed alevesely closed 8 years ago

alevesely commented 8 years ago

This error doesn't seem to cause any malfunction, except being reported as such. On the console, I get:

Error: 2015-11-04 13:09:55 DKIM_Verifier.AuthVerifier ERROR Error: Parsing error (resource://dkim_verifier/ARHParser.jsm:249:2) JS Stack trace: match@ARHParser.jsm:249:3 < parseResinfo@ARHParser.jsm:186:2 < _ARHParser_parse@ARHParser.jsm:161:4 < getARHResult@AuthVerifier.jsm:161:4 < _authVerifier_verify/promise<@AuthVerifier.jsm:103:4

The offending header was:

Authentication-Results: authserv-id;
    dnswl=pass dns.zone=list.dnswl.org
    policy.ip=127.0.2.3
    policy.txt="example.com http://dnswl.org/s?s=2207"

By browsing, it seems to me the parser is compliant with RFC 5451. That is, it only accepts ptypes smtp|header|body|policy, throwing a syntax error if no match is found.

RFC 7410 relaxed that constrain and started an IANA registry. Yet, the above ptypes are missing from that registry as well. Courier-MTA sets them if configured to do so, but they are not documented (policy.ip is documented in https://www.dnswl.org/?page_id=15#returncodes). While coloring/linking dnswl data would be fancy, turning the error into a warning is certainly enough to fix this issue.

lieser commented 8 years ago

Thanks for reporting it. I will update the parser to be compliant with RFC 7601. According to it an unknown ptype should be ignored. It is unlikely that I will add support for the "dnswl" method.

I will probably also change how a parsing error in the ARH-header is treated. Currently it simple stops the verification without showing that an error occurred (besides the logging in the console). It should either inform the user that an error occurred, or ignore the ARH-header and make a local verification of the DKIM signature.

alevesely commented 8 years ago

It should either inform the user that an error occurred, or ignore the ARH-header and make a local verification of the DKIM signature.

The choice should depend on who wrote the header. Some servers don't allow injecting spurious Authentication-Results fields. Otherwise, their validity should be inferred from authserv-id. I suppose users who connect to multiple servers will have to manually configure what to do with each.

lieser commented 8 years ago

I think you misunderstood me. I was talking only about parsing errors, which could occur if one of the following conditions apply:

An injected and spurious ARH header would not necessarily trigger such an error. So showing or ignoring this kinds of errors would not help preventing the injection. What would help is the ability to enable/disable the reading of the ARH header for each account (and disable it for servers who on does not trust). You suggestion about comparing the authserv-id with a list of trusted server would also be helpful.

alevesely commented 8 years ago

No misunderstandings. I was just trying to prevent you from warning users about syntax errors in untrusted headers.

Keep up the good work!

lieser commented 8 years ago

The ARH parser now also allows unknown property types. With this, the ARH parser should now be compliant with the RFC 7601. You can download a pre-release from https://github.com/lieser/dkim_verifier/releases.

I have created separated issues (#54, #55) for the discussed options .

lieser commented 8 years ago

Just wanted to let you know that both discussed options (enabling ARH per account and only trusting specific servers) will be available in the next version.