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

Status: RESOLVED FIXED

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

Description

[Daniel Stenberg [:bagder]]

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 1

[Daniel Stenberg [:bagder]]

Attachment: bug 1451928 - loop detection added for the TRR CNAME parser

Review commit: https://reviewboard.mozilla.org/r/234282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234282/

Comment 2

[Valentin Gosu [:valentin] (he/him)]

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?

Comment 3

[Daniel Stenberg [:bagder]]

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.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

(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!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c587348ba99b
loop detection added for the TRR CNAME parser r=valentin

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/c587348ba99b