Closed Bug 1451928 Opened 2 years ago Closed 2 years ago

TRR: a crafted CNAME response can cause an endless busy-loop

Categories

(Core :: Networking: DNS, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bagder, Assigned: bagder)

Details

(Whiteboard: [necko-triaged][trr])

Attachments

(1 file)

The TRR:DohDecode() "follows" pointers in the DNS response packet when it decodes CNAME responses, and if when following a pointer, a subsequent pointer could point back to the original position (or similar) and the decoder would get stuck in a loop.

A loop detector is necessary.
Comment on attachment 8965500 [details]
bug 1451928 - loop detection added for the TRR CNAME parser

https://reviewboard.mozilla.org/r/234282/#review240316

::: netwerk/dns/TRR.cpp:638
(Diff revision 1)
>              // name pointer, get the new offset (14 bits)
>              if ((cindex +1) >= mBodySize) {
>                return NS_ERROR_ILLEGAL_VALUE;
>              }
>              // extract the new index position for the next label
>              uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1];

I'm assuming we can't require that newpos is always bigger than cindex?
Attachment #8965500 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8965500 [details]
bug 1451928 - loop detection added for the TRR CNAME parser

https://reviewboard.mozilla.org/r/234282/#review240316

> I'm assuming we can't require that newpos is always bigger than cindex?

No we can't. That's not how the format works, and it is actually most often used to point back to a previously used name.
(In reply to Daniel Stenberg [:bagder] from comment #3)
> > I'm assuming we can't require that newpos is always bigger than cindex?
> 
> No we can't. That's not how the format works, and it is actually most often
> used to point back to a previously used name.

That's fair :) Let's ship it!
Pushed by daniel@haxx.se:
https://hg.mozilla.org/integration/autoland/rev/c587348ba99b
loop detection added for the TRR CNAME parser r=valentin
https://hg.mozilla.org/mozilla-central/rev/c587348ba99b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.