bounds-check dns response parsing against packet end

[rootvector2]

parseQname follows a compression pointer and copies label bytes with no packet-end check, and the TXT loop in parseQuestionOrAnswer underflows rdlength when a sub-string length exceeds it, so a malformed DNS response read from the resolver's UDP socket reads past the packet buffer.

[rootvector2]

@phoddie

[phoddie]

Thank you for the PR.

• It identifies some memory safety issues. That's great.
• It missed some others - most notably the xs_dnspacket_get_* functions don't validate that the header is fully present.
• Some of the changes to optimize refresh of the packet pointer will break, because packet can move during certain operations
• Some of fixes are more invasive / complicate than strictly necessary.

I drafted an alternative solution. You can see that as a comparison with the current Moddable SDK version in this gist.

[rootvector2]

you're right on all three. the refresh optimization is the real bug: xsCall1(..., xsID_push, ...) can move the backing store, so caching packet across the label loop leaves position pointing into freed memory. i was treating the buffer as fixed, which it isn't.

centralizing the < 12 check in getPacket is cleaner than guarding each xs_dnspacket_get_*, and the while (position < packetEnd) conditions read better than the explicit early returns i added.

your draft covers everything mine does plus the header case. happy to defer to it. want me to push it to this branch, or will you land it directly?

[phoddie]

Thanks for reviewing. If you want to update the PR, that would be fine. You started the effort.

[rootvector2]

pushed your draft to the branch, squashed into the one commit so the diff stays just the bounds checks. the < 12 check in getPacket and re-reading packet/packetEnd after each getPacket cover the header and pointer-move cases mine missed. reads clean to me.