Make it a non-fatal error when EncodeGrab fails to marshal data #355
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
As called by grabTarget. The proximate cause for this is zmap#318 (SSH certificates with dates too large crash the process), and the fix is due to @TrueSkrillor in that same issue. The source of the error is the go standard library's time module (https://cs.opensource.google/go/go/+/master:src/time/time.go;drc=41b9d8c75e45636a153c2a31d117196a22a7fc6c;l=1317), and it points us to golang/go#4556 (comment). Essentially, time is trying to produce a RFC3339 string, and refuses to do so with a year too big. We began seeing this problem about the week of June 6, 2022, and this change fixed it for us. However, this does appear to be a global change, applying to all scanners. We aren't currently affected by this (or we would have had other uses of zgrab2 crashing with a similar error), but in the future, should we hit bad data, we won't crash, we'll just get a logged warning that we may not notice. That's both good and bad. I don't know go well. I was a bit concerned about passing what must be the nil return value back from grabTarget, into the outputQueue byte[] channel, and ultimately into the OutputResults function which wants to write it to the (buffered) output file. However, it appears that, because len(v), where v is a byte[], is 0, nothing actually happens: the empty result is swallowed.
dadrian
approved these changes
Jan 24, 2023
|
Thanks! |
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.
Make it a non-fatal error when
EncodeGrabfails to marshal data when called bygrabTarget. Previously, if a result couldn't be encoded, the process crashed. Now, it logs an error and continues.The proximate cause for this is #318 (SSH certificates with dates too large crash the process), and the workaround is due to @TrueSkrillor in that same issue. The source of the error is the go standard library's time module, and it points us to golang/go#4556 (comment). Essentially, time is trying to produce a RFC3339 string, and refuses to do so with a year that's out of range.
We began seeing this problem about the week of June 6, 2022, and this change got us past that problem.
How to Test
The most direct test would be to scan a server vending certificates that trigger this problem. That's potentially hard because I can't provide a list of such servers (they weren't consistent from one day to the next in our testing). Otherwise, I'm not sure how to get the invalid data into
grabTarget; I admit I haven't looked at the test suite for this project at all (sorry!).Notes & Caveats
However, this does appear to be a global change, applying to all scanners. We aren't currently affected by this (or we would have had other uses of zgrab2 crashing with a similar error), but in the future, should we hit bad data, we won't crash, we'll just get a logged warning that we may not notice. That's both good and bad, and I certainly understand if this doesn't get merged because of its global nature.
I don't know go well. I was a bit concerned about passing what must be the
nilreturn value back fromgrabTarget, into theoutputQueuebyte[]channel, and ultimately into theOutputResultsfunction which wants to write it to the (buffered) output file. However, from reading the go stdlib's source, it appears that, becauselen(v), where v is abyte[], is 0, nothing actually happens: the empty result is swallowed by the buffered writer.Issue Tracking
#318