492717 - nsNTLMAuthModule.cpp ParseType2Msg(): need to check |offset| value

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[georgi - hopefully not receiving bugspam]

this seems currently dead code but seems wrong by inspection.

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNTLMAuthModule.cpp#547

547   PRUint32 offset = ReadUint32(cursor); // get offset from inBuf
548   msg->target = ((const PRUint8 *) inBuf) + offset;

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> this seems currently dead code

What makes you think that?

Comment 2

[georgi - hopefully not receiving bugspam]

> What makes you think that?

i suspect |msg->target| is not used, this is what i mean. not sure about it.

Comment 3

[Serge Gautherie (:sgautherie)]

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNTLMAuthModule.cpp?mark=513-514,544-545,547-548,559-559,564-566#547
{
518 ParseType2Msg(const void *inBuf, PRUint32 inLen, Type2Msg *msg)
...
564  // we currently do not implement LMv2/NTLMv2 or NTLM2 responses,
565  // so we can ignore target information. we may want to enable
566  // support for these alternate mechanisms in the future.
}
then
{
571 GenerateType3Msg(const nsString &domain,
...
584   rv = ParseType2Msg(inBuf, inLen, &msg);
...
}
currently uses |msg.flags| and |msg.challenge| only.

So the |target*| are indeed unused "yet": no actual security issue atm.
(But it doesn't look like dead/removable code, per the quoted comment.)

***

(In reply to comment #0)
> this [...] seems wrong by inspection.

What do you think is wrong in this code?
Would you know what a better code would be?
Would the issue really be x86/Linux specific or rather all/all (or Windows only)?
Would you have an example of log output which would show the problem?

Comment 4

[georgi - hopefully not receiving bugspam]

>What do you think is wrong in this code?

|offset| can be any int32 value afaict.

this makes |target| any pointer on 32 bit systems and a lot of pointers on 64 bit systems.

Comment 5

[georgi - hopefully not receiving bugspam]

so if |target| is used with attacker supplied |offset| this may be a problem - at least an easy crash.

Comment 6

[Serge Gautherie (:sgautherie)]

Ah! This makes sense now:
this calculation itself is actually right,
but we miss to check |offset| value before using it.

Comment 7

[georgi - hopefully not receiving bugspam]

huh, how is this critical crash if the wild pointer is not used?

Comment 8

[Serge Gautherie (:sgautherie)]

I guess we should check |msg->targetLen| value too...


(In reply to comment #7)
> huh, how is this critical crash if the wild pointer is not used?

It's not actively used in the authentication process,
but it is used by
{
145 #ifdef PR_LOGGING
...
205   if (!LOG_ENABLED())
...
559 LogBuf("target", (const PRUint8 *) msg->target, msg->targetLen);
}

This is already a case, even if rare,
but this bug is about fixing this code before it (would) eventually gets a more general use, isn't it?

Comment 9

[georgi - hopefully not receiving bugspam]

sure it better be fixed, this is why i filed it ;)

Comment 10

[georgi - hopefully not receiving bugspam]

in theory if target is exposed to userland this may expose memory contents even without writing or dereferencing content from it.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Chromium folks just patched this:
http://codereview.chromium.org/155311

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) offset and length [Checkin: Comment 17 & 20]

Back-Port Chromium patch.

Fwiw, see http://davenport.sourceforge.net/ntlm.html#theType2Message

Comment 13

[Robert Relyea]

Adding module owner.

Comment 14

[Robert Relyea]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]

r+ rrelyea

Code is correct. so I'm giving this a r+ so as not to hold up a security fix any longer.

bob

Comment 15

[Kai Engert (:KaiE:)]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]

thanks, looks good to me. r=kaie

Comment 16

[Reed Loden [:reed]]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]

rrelyea and kaie's review should be enough... Ready to land.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]


http://hg.mozilla.org/mozilla-central/rev/d1c0b2c4ac7a

***

"approval1.9.1.2=?":
No risk, just to be future-proof.

Comment 18

[Samuel Sidler (old account; do not CC)]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]

Not for 1.9.1.2.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 388643 [details] [diff] [review]
(Av1) offset and length
[Checkin: Comment 17 & 20]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dbcc002ebde8

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified both fixes based on checkins on 1.9.2 and 1.9.1.