DecodeJSON accepts "{}\n some garbage data" incorrectly

[earthboundkid]

Because json.NewDecoder expects to decode a stream of objects, it lets malicious clients add garbage data to the end of a request, which may lead to a "confused deputy" security bug.

See golang/go#36225.

[VojtechVitek]

For reference:

@atticus-sullivan atticus-sullivan 4 hours ago
Not completely sure but as I see it, not using the Decoder/Encoder requires a double-pass over the data (1. collecting the []byte from the io.reader and 2. later parse the []byte to struct). So in general I'm in favor of using Decoder/Encoder. Nevertheless, if this additional data which goes undetected is a security issue we should definitely avoid it. But that's the point I'm not sure about.

Doesn't it help explicitly discarding everything which comes after the parsed JSON object?

[atticus-sullivan]

Ok, just to give some context. I'm not completely sure what exactly the problem is @carlmjohnson addressed.
In general I see two issues:

1. An attacker can inject arbitrary data (like shell commands) after the JSON object which could be used in another attack (needs something like a Buffer Overflow or so I think).
2. Not a concrete issue, but being able to add arbitrary data in the end always makes length extension like attacks possible (trying to e.g. find a hash collision or calculating a valid keyed hash without knowing the key). But again I think this would need another vulnerability (like building the keyed hash the wrong way) to be present. Nevertheless, the ability to inject arbitrary data is missing input validation.

So I'd like to change my overall statement that this is no problem into I think it's not that severe for now, but we should do something about it (especially as we're working on it right now anyhow).

I see two possibilities:

1. Manually check if the reader is at EOF (read of one byte and check if it returned io.EOF) after the Decode
2. switch to unmarshal. This requires us to read the data first and decode afterwards. I always think that's not so nice as we need two passes over the data here, but I never benchmarked it to see if it's really that bad (speaking of runtime complexity it is only a constant factor).

And in the end when encoding/json/v2 is out we can just use this. But the issue carlmjohnson linked is marked as unplanned, so this might take a while.

---

Do you see any other issues regarding the current state? (I learned a bit about security, but I'm not a security expert so I might have missed something or put something wrong) Maybe @carlmjohnson can add more details to the initial thought.

What do you think we should do now?

[earthboundkid]

Yes, per se this isn’t so bad. It mostly just hides bugs when you accidentally send two objects. But it makes it easier to do a confused deputy attack, so it’s not great in that regard, because it leaves out a layer of depth in defense. I think the easiest solution is just reading a second object after the end of the first decoder return and ensuring that it’s an EOF error.

[VojtechVitek]

Since we're discarding anything after the first JSON object, I think it's safe to close this issue.

render/decoder.go

Line 42 in 14f1cb3

defer io.Copy(io.Discard, r) //nolint:errcheck