334185 - double free in HandshakeCallback if CERT_GetOrgName fails

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

Comment 1

[timeless]

Attachment: don't double destroy signer

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't follow.  The comment and assert seem clear; are they wrong?

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 218646 [details] [diff] [review]
don't double destroy signer

The double free case is:
- signer could be allocated
- caName could not be allocated

In this case caName will be set to point to the same memory as signer.
The existing code will free caName == signer, and again free signer at the end of the routine.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 218646 [details] [diff] [review]
don't double destroy signer

In that case we should fix the comment and remove the assert.

Comment 5

[chris hofmann]

should this still be sg:critical?

Comment 6

[Daniel Veditz [:dveditz]]

Kai: is this a bug or not? Boris points out the inconsistency between the comment and the code change. If we need this please fix the comment and shepherd this in.

Comment 7

[Kai Engert (:KaiE:)]

Let's get this right.

First, this code is a nightmare. It's difficult to read and understand. I will propose a patch that makes it all more obvious.

But before I do, let me explain the comment and the assertion.

The existing code plays games with pointer variable caName.
It may point to a string that must get freed, or a string that must not get free.

The comment (that Boris complains about) is meant to make it easier to understand the code.

The means: If caName points to a string equal to "RSA...", then caName must origin from CERT_GetOrgName. caName can not be signer. Because signer would be a X.509 name, where all parts are prefixed. Because caName is equal to "RSA...", it does not start with "O=RSA...". But if the caName had its origin in signer, then the string would have to look like "O=RSA...".

So the code concludes it can not be signer, it must origin from CERT_GetOrgName (which we no longer need) and its therefore safe to free the current caName pointer.


Having read this in more detail, and assuming the comment is right, this crash is purely hypothetical. Because if we are sure that caName has its origin in CERT_GetOrgName, it obviously got allocated...


I propose we avoid future confusion about this code.
I will attach a patch that makes it more obvious.

I don't want to proof whether the assumption about a O= prefix is right.
I propose we change the comment into a hint that "probably" caName has its origin in CERT_GetOrgName, and remove the statement about freeing memory, and 
remove the assertion.


Timeless, did you actually run into this crash?

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265672 [details] [diff] [review]
Patch v2

>Index: mozilla/security/manager/ssl/src/nsNSSCallbacks.cpp
>+    // In this case, caName should have its origin in certOrgName.
>+    // The signer string would be at minimal "O=RSA Data Security, Inc".

Why does any of that matter?  In this case we just repoint our pointer; what do we care where it comes from?

>     caName = PL_strdup("Verisign, Inc.");

fwiw, if we just make this a string constant instead of calling strdup, can't we just drop verisignName altogeter?

sr=bzbarsky with the nits addressed.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v3

- Comment removed
- strdup eliminated
- converted verisign name into a constant string

Comment 11

[Robert Relyea]

Comment on attachment 265729 [details] [diff] [review]
Patch v3

r+ The new code is much easier to prove correct.

bob

Comment 12

[Kai Engert (:KaiE:)]

Patch checked in to trunk.

Comment 13

[Daniel Veditz [:dveditz]]

Fixed on trunk, worth backporting to the 1.8 branch?