Add a hook to catch invalid messages #1568
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently there are hooks for reading messages off the wire (DecorateReader), checking if they comply with policy (MsgAcceptFunc), and generating responses (Handler). However, there is no hook that notifies the server when a message is dropped or rejected due to a syntax error. That makes it hard to monitor these packets without repeating the parsing process. This PR adds a hook for notifications about invalid packets.
miekg
reviewed
Jun 13, 2024
|
excellent idea. Small comment and making the names more consistent |
jefferai
pushed a commit
to jefferai/dns
that referenced
this pull request
Aug 7, 2024
* Swap closing order in `inAxfr` and `inIxfr` (miekg#1511) * Fix closing order * Comment to make clear that the close order is deliberate --------- Co-authored-by: Tim Scheuermann <tscheuermann@anexia-it.com> * feat: add support for ReuseAddr (miekg#1510) * feat: add support for ReuseAddr * Update listen_reuseport.go * Update listen_reuseport.go * fixup! feat: add support for ReuseAddr --------- Co-authored-by: Miek Gieben <miek@miek.nl> * Release 1.1.57 * Try explaining duplicate RCODEs Add extra link to the docs for the duplicate Rcode entries See miekg#1523 Signed-off-by: Miek Gieben <miek@miek.nl> * docs: added ninedos to readme (miekg#1522) * Allow use of fs.FS for $INCLUDE and wrap errors (miekg#1526) * Allow use of fs.FS for $INCLUDE and wrap errors This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when enabling support for $INCLUDE, for reading included files from somewhere other than the local filesystem. I've also modified ParseError to support wrapping another error, such as errors encountered while opening the $INCLUDE target. This allows for much more robust handling, using errors.Is() instead of testing for particular strings (which may not be identical between fs.FS implementations). ParseError was being constructed in a lot of places using positional instead of named members. Updating ParseError initialization after the new member field was added makes this change seem a lot larger than it actually is. The changes here should be completely backwards compatible. The ParseError change should be invisible to anyone not trying to unwrap it, and ZoneParser will continue to use os.Open if the existing SetIncludeAllowed method is called instead of the new SetIncludeAllowedFS method. * Don't duplicate SetIncludeAllowed; clarify edge cases Rather than duplicate functionality between SetIncludeAllowed and SetIncludeAllowedFS, have a method SetIncludeFS, which only sets the fs.FS. I've improved the documentation to point out some considerations for users hoping to use fs.FS as a security boundary. Per the fs.ValidPath documentation, fs.FS implementations must use path (not filepath) semantics, with slash as a separator (even on Windows). Some, like os.DirFS, also require all paths to be relative. I've clarified this in the documentation, made the includePath manipulation more robust to edge cases, and added some additional tests for relative and absolute paths. * Bump golang.org/x/net from 0.17.0 to 0.19.0 (miekg#1520) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.17.0 to 0.19.0. - [Commits](golang/net@v0.17.0...v0.19.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/sys from 0.13.0 to 0.15.0 (miekg#1518) Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.13.0 to 0.15.0. - [Commits](golang/sys@v0.13.0...v0.15.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Add NXT record (miekg#1516) This add the NXT record (2535) to implement all records from the RFC. Also does a s/RFC RFC/RFC/ as I happen to bumb into that will editing the comments. Signed-off-by: Miek Gieben <miek@miek.nl> * Add ISDN record (miekg#1515) We had the type code, this add the rest. Other RRs from 1183 are also fully impl. don't know why this one wasn't. Signed-off-by: Miek Gieben <miek@miek.nl> * Bump golang.org/x/tools from 0.13.0 to 0.17.0 (miekg#1529) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.13.0 to 0.17.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.13.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Release 1.1.58 * Improve NewRR documentation (miekg#1531) In particular, document the default origin. * Add incus to the list of users (miekg#1535) * Add option to do a zone transfer via TLS (miekg#1533) * New func InTLS Perform zone transfer via TLS * Test xfr via TLS * New field TLS, used to transfer via TLS --------- Co-authored-by: Cesar Kuroiwa <cesar@registro.br> * IsDomainName: check for escape as last character (miekg#1532) Keep track if the escape, if still true when returning isDomainName should return false. TODO: - Should still be done in packDomainName as well. - And that should be tested - Some tests now fail There are multiple other places that supposedly also check for this, but they are not called in the parsing. Fixes: miekg#1528 Signed-off-by: Miek Gieben <miek@miek.nl> * Bump golang.org/x/sys from 0.16.0 to 0.17.0 (miekg#1541) Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.16.0 to 0.17.0. - [Commits](golang/sys@v0.16.0...v0.17.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/net from 0.20.0 to 0.21.0 (miekg#1542) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.20.0 to 0.21.0. - [Commits](golang/net@v0.20.0...v0.21.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/tools from 0.17.0 to 0.19.0 (miekg#1551) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.17.0 to 0.19.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.17.0...v0.19.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: fix some comments (miekg#1547) Signed-off-by: xiaoxiangxianzi <zhaoyizheng@outlook.com> * Add ifconfig.es to the list of users (miekg#1554) * Fix counting of escape sequences when splitting TXT strings (miekg#1540) `endingToTxtSlice`, used by TXT, SPF and a few other types, parses a string such as `"hello world"` from an RR's content in a zone file. These strings are limited to 255 characters, and `endingToTxtSlice` automatically splits them if they're longer than that. However, it didn't count the length correctly: escape sequences such as `\\` or `\123` were counted as multiple characters (2 and 4 respectively in these examples), but they should only count as one character because they represent a single byte in wire format (which is where this 255 character limit comes from). This commit fixes that. * Fix possible out-of-bounds read in endingToTxtSlice (miekg#1557) * Update escapedStringOffset to improve readability This function was, admittedly, a little difficult to follow. This new version is slightly more verbose, but, in my opinion, easier to understand. * Fix possible out-of-bounds read in endingToTxtSlice caused by escapedStringOffset If the input had a trailing backslash (normally the start of an escape sequence) with nothing following it, `escapedStringOffset` would return the length of the input, plus one (!), as the result index, causing an out-of-bounds read and panic in `endingToTxtSlice`. Consistent with, e.g., commit 2230854, I've decided to make this an error since it definitely indicates that the string isn't valid. Credit to OSS-Fuzz -- thank you! * Bump golang.org/x/sys from 0.18.0 to 0.20.0 (miekg#1571) Bumps [golang.org/x/sys](https://github.com/golang/sys) from 0.18.0 to 0.20.0. - [Commits](golang/sys@v0.18.0...v0.20.0) --- updated-dependencies: - dependency-name: golang.org/x/sys dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/net from 0.22.0 to 0.25.0 (miekg#1569) Bumps [golang.org/x/net](https://github.com/golang/net) from 0.22.0 to 0.25.0. - [Commits](golang/net@v0.22.0...v0.25.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump golang.org/x/tools from 0.19.0 to 0.22.0 (miekg#1574) Bumps [golang.org/x/tools](https://github.com/golang/tools) from 0.19.0 to 0.22.0. - [Release notes](https://github.com/golang/tools/releases) - [Commits](golang/tools@v0.19.0...v0.22.0) --- updated-dependencies: - dependency-name: golang.org/x/tools dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * (*Transfer) Out: Increment WaitGroup in example (miekg#1572) * Add a hook to catch invalid messages (miekg#1568) * Add a hook to catch invalid messages Currently there are hooks for reading messages off the wire (DecorateReader), checking if they comply with policy (MsgAcceptFunc), and generating responses (Handler). However, there is no hook that notifies the server when a message is dropped or rejected due to a syntax error. That makes it hard to monitor these packets without repeating the parsing process. This PR adds a hook for notifications about invalid packets. * s/InvalidMsg/MsgInvalid/g * These two too Signed-off-by: Miek Gieben <miek@miek.nl> * Add RFC 9540 oblivious services via service binding records (miekg#1567) * update list of RFCs Signed-off-by: Miek Gieben <miek@miek.nl> * add rfc3596 to the list (miekg#1577) --------- Signed-off-by: Miek Gieben <miek@miek.nl> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: xiaoxiangxianzi <zhaoyizheng@outlook.com> Co-authored-by: Tim Scheuermann <noxer@users.noreply.github.com> Co-authored-by: Tim Scheuermann <tscheuermann@anexia-it.com> Co-authored-by: Jim <jlambert@hashicorp.com> Co-authored-by: Miek Gieben <miek@miek.nl> Co-authored-by: WintBit <wintbit@gmail.com> Co-authored-by: Dave Pifke <dave@pifke.org> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Richard Gibson <richard.gibson@gmail.com> Co-authored-by: montag451 <montag451@laposte.net> Co-authored-by: Cesar Kuroiwa <abc@kuroiwa.com.br> Co-authored-by: Cesar Kuroiwa <cesar@registro.br> Co-authored-by: xiaoxiangxianzi <164908047+xiaoxiangxianzi@users.noreply.github.com> Co-authored-by: dcarrillo <daniel.carrillo@gmail.com> Co-authored-by: Janik Rabe <janik@cloudflare.com> Co-authored-by: Patrik Lundin <patrik@sigterm.se> Co-authored-by: Benjamin M. Schwartz <ben@bemasc.net> Co-authored-by: Steffen Sassalla <32709406+steffsas@users.noreply.github.com> Co-authored-by: Infinoid <mark-github@glines.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently there are hooks for reading messages off the wire (DecorateReader), checking if they comply with policy (MsgAcceptFunc), and generating responses (Handler). However, there is no hook that notifies the server when a message is dropped or rejected due to a syntax error. That makes it hard to monitor these packets without repeating the parsing process.
This PR adds a hook for notifications about invalid packets.