MiniDNS / minidns

DNS library for Android and Java SE
Other
215 stars 61 forks source link

DANE verification broken in multiple ways even on cursory inspection. #105

Open vdukhovni opened 3 years ago

vdukhovni commented 3 years ago

This implementation is sufficiently naïve and incomplete to warrant strong warnings that the code SHOULD NOT be trusted to avoid either false positives or false negatives. It is still an early prototype not ready for primetime.

Flowdalic commented 3 years ago

Thanks for your feedback Viktor.

I think it would be sensible to categorize the points you mentioned in "security-related" and "missing functionality" ones.

If I am not mistaken, and please correct me if I am wrong, from the four points you bring up, CNAME chasing and digest algorithm agility belong into the "missing functionality" category.

That remaining ones are

  1. Trust manager sometimes uses insecurely derived hostnames
  2. PKIX-TA(0) and DANE-TA(2) are not handled correctly

Looking at the code, MiniDNS does not support PKIX-TA and DANE-TA, in both cases the verification fails, and a warning is logged, noting that this is not (yet) supported. Hence I tend to categorize this as "missing functionality". However, you write that "PKIX-TA(0) and DANE-TA(2) are not handled correctly", which reads differently than my assessment.

Without having had time for a closer look at 1) and 2) and giving it more though, would you mind to elaborate a little bit? For example, some code pointers where the trust manager sometimes uses hostnames in an insecure way would help a lot.

vdukhovni commented 3 years ago

Which brings me to additional points:

The current code performs no namechecks for DANE-EE(3), but that turns out to not always be the right thing to do.

Take a look at the OpenSSL DANE documentation for some idea of what sort of tunable parameters might be appropriate.

And this isn't a complete review. The code is not ready, and should not be used.

vdukhovni commented 3 years ago

Also, when DANE validation completes, callers should be able to get at the matching TLSA record, and matching certificate for diagnostic purposes, although a sufficiently fancy logging framework can make some of that less critical, if all they want is to display it to a user or record the information in the log.

vdukhovni commented 3 years ago

More seriously perhaps, also the DNSSEC client does a poor job of validation of denial of existence:

Writing a modern DNS resolver is rather difficult. The concerns are many and subtle. :-(