Validate offset / length of extra field in the message. Note these fields are...

Validate offset / length of extra field in the message. Note these fields are
decoded and set in a structure but unsused, so this is not a current security
issue. This change just future-proofs the area in case these fields are used
one day.

BUG=NONE
TEST=NONE


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20315

[eroman, 2009-07-09 20:54:47]

LGTM.

+wtc

[wtc, 2009-07-13 18:07:30]

We should submit a patch to the Mozilla upstream.

http://codereview.chromium.org/155311/diff/1/2
File net/http/http_auth_handler_ntlm.cc (right):

http://codereview.chromium.org/155311/diff/1/2#newcode421
Line 421: if (offset + target_len > offset && offset + target_len <= in_len) {
Should we return ERR_UNEXPECTED or some other error code if
this is false?

[cevans, 2009-07-13 18:24:35]

On Mon, Jul 13, 2009 at 11:07 AM, <wtc@chromium.org> wrote:

> We should submit a patch to the Mozilla upstream.


I didn't see that code in Mozilla upstream, but I only looked in
nsHttpNTLMAuth.cpp. Let me know if there's another file I should check.


>
>
> http://codereview.chromium.org/155311/diff/1/2
> File net/http/http_auth_handler_ntlm.cc (right):
>
> http://codereview.chromium.org/155311/diff/1/2#newcode421
> Line 421: if (offset + target_len > offset && offset + target_len <=
> in_len) {
> Should we return ERR_UNEXPECTED or some other error code if
> this is false?


I was worried about the possibility (particularly since older Microsoft
operating systems will be generating these messages) of some implementations
generating rubbish in these fields.

Cheers
Chris


>
>
> http://codereview.chromium.org/155311
>

[wtc, 2009-07-14 00:18:49]

On 2009/07/13 18:24:35, cevans wrote:
>
> I didn't see that code in Mozilla upstream, but I only looked in
> nsHttpNTLMAuth.cpp. Let me know if there's another file I should check.

That code is in
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNTLM...
lines 544-548.

> I was worried about the possibility (particularly since older Microsoft
> operating systems will be generating these messages) of some implementations
> generating rubbish in these fields.

Then perhaps we can just delete the code that assigns
values to msg->target and msg->target_len, and remove
the target and target_len members from struct Type2Msg.

We can add a comment to explain that we allow rubbish
in these fields for backward compatibility.

[cevans, 2009-07-14 00:49:47]

On Mon, Jul 13, 2009 at 5:18 PM, <wtc@chromium.org> wrote:

> On 2009/07/13 18:24:35, cevans wrote:
>
>  I didn't see that code in Mozilla upstream, but I only looked in
>> nsHttpNTLMAuth.cpp. Let me know if there's another file I should
>>
> check.
>
> That code is in
>
>
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNTLM...
> lines 544-548.


I'll look at this and talk to my Mozilla security contacts as appropriate :)


>
>  I was worried about the possibility (particularly since older
>>
> Microsoft
>
>> operating systems will be generating these messages) of some
>>
> implementations
>
>> generating rubbish in these fields.
>>
>
> Then perhaps we can just delete the code that assigns
> values to msg->target and msg->target_len, and remove
> the target and target_len members from struct Type2Msg.
>
> We can add a comment to explain that we allow rubbish
> in these fields for backward compatibility.
>
>
> http://codereview.chromium.org155311<http://codereview.chromium.org/155311>
>

Do you think we might ever want to look at these fields? If so, I'd say
leave the code, so that it's safe for the future. A reimplementation might
get it wrong again.

Cheers
Chris