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

TLSA cert validation #257

Closed
andrew-boutin opened this issue Jun 22, 2022 · 5 comments
Closed

TLSA cert validation #257

andrew-boutin opened this issue Jun 22, 2022 · 5 comments
Milestone

Comments

@andrew-boutin
Copy link

Some rdata for TLSA records, specifically cert values, are allowed that I believe should be rejected per https://datatracker.ietf.org/doc/html/rfc6698#section-2.1.

  • Missing cert.
  • Non-hex char "D6FCE13243AA7-". This contains a '-' that should be rejected. "D6FCE13243AAZ" was rejected correctly which contains 'Z'.
  • Too long. I'm not quite sure what the max should be, but I was able to create the record using over 10k As in a row.
@ibauersachs
Copy link
Member

Thanks! Against which version of dnsjava did you test this? The changes from #252 (released in v3.5.1) also fixed points 1 and 2.

I'm not sure about the third point, max length. Do you see a use case where this is really an issue?

@andrew-boutin
Copy link
Author

I was on 3.2.2, but I just tested again on 3.5.1.

I get a NullPointerException when testing missing cert using rdata "0 0 1".

Caused by: java.lang.NullPointerException
	at org.xbill.DNS.utils.base16.toString(base16.java:30)
	at org.xbill.DNS.TLSARecord.rrToString(TLSARecord.java:137)
	at org.xbill.DNS.Record.rdataToString(Record.java:278)

I don't see any errors for non-hex char not a letter. For example, "3 1 1 0D6FCE13243AA-". Per 182b5a3#diff-f9d43ac9568da097f6fcf1841a792ae3168643498a01ead4a638d4dc5cb65d23R39 it looks like maybe this is by design? If so, why reject non-hex chars when using a letter not in A-F, but not reject other characters? For example, "3 1 1 D6FCE13243AAZ" is rejected.

For the last bullet point, I'm not sure. I was thinking that if something was relying on the dnsjava lib validation that this would help prevent storing extremely long strings that would bloat a database. May help prevent malicious behavior.

@ibauersachs
Copy link
Member

ibauersachs commented Oct 3, 2022

Are you sure you tested this against 3.5.1? Can you please post a complete example?

When I try to parse 0 0 1 then a TextParseException is thrown. However, an empty byte[] when using the constructor is currently not correctly handled (producing an invalid string, requires the same fix as #256 if this should be allowed at all).

ibauersachs added a commit that referenced this issue Oct 9, 2022
@andrew-boutin
Copy link
Author

@ibauersachs I re-tested on 3.5.1 and the missing cert and non-hex char - are being handled correctly. Sorry, the version was updated in my pom.xml when I tested last, but maybe the dependency hadn't been pulled down yet or something. Thanks again for improving all of that validation.

I still think it would be good to protect from maliciously long inputs whenever possible.

@ibauersachs ibauersachs modified the milestone: v3.5.2 Dec 24, 2022
@ibauersachs
Copy link
Member

I'm closing this. I couldn't find anything in the RFC that limits the association data length. As whitespace is allowed, a text-message of ABC [100k spaces] DEF is still valid. Over the wire, the length is limited to the max size of a DNS message, 2^16, which I don't consider an issue.

@ibauersachs ibauersachs added this to the v3.5.2 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants