737395 - SECITEM_CompareItem() returns invalid result, found with memory analysis tool that uses its own copy of "memcmp"

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kai Engert (:KaiE:)]

Originally reported at https://bugzilla.redhat.com/show_bug.cgi?id=804802
by John Dennis. Copied from the original report:


SECITEM_CompareItem() 

Look at nss/lib/util/secitem.c:153

and you'll see SECITEM_CompareItem is defined to return a SECComparison
enumerated value

SECComparison
SECITEM_CompareItem(const SECItem *a, const SECItem *b)

This is the definition of SECComparision:

typedef enum _SECComparison {
    SECLessThan = -1,
    SECEqual = 0,
    SECGreaterThan = 1
} SECComparison;

Thus only SECLessThan, SECEqual or SECGreaterThan should ever be returned
(-1,0,1 respectively)

But these lines of code in SECITEM_CompareItem() will produce an invalid value
to be returned.

    rv = (SECComparison) PORT_Memcmp(a->data, b->data, m);
    if (rv) {
 return rv;
    }

That is because memcmp is defined according to POSIX:

------

The sign of a non-zero return value shall be determined by the sign of the
difference between the values of the first pair of bytes (both interpreted as
type unsigned char) that differ in the objects being compared.

RETURN VALUE

The memcmp() function shall return an integer greater than, equal to, or less
than 0, if the object pointed to by s1 is greater than, equal to, or less than
the object pointed to by s2, respectively.

------

That means when using memcmp you MUST examine at the SIGN of the return value,
not the return value. Some memcmp implementations return the signed difference
between the first non-equal octets instead of -1 or 1.

Thus the above code needs to modified thusly:

    int rv;

    ...

    rv = PORT_Memcmp(a->data, b->data, m);
    if (rv) {
 return rv < 0 ? SECLessThan : SECGreaterThan;
    }

The problem manifests itself because there are several places in the code where
the result of SECITEM_CompareItem() is directly compared to the enumerated
constants SECLessThan or SECGreaterThan. It's also logically incorrect because
it's not returning one of the enumerated values.

BTW, this is causing the python-nss unit tests to fail. We need to release a
new version of python-nss but it's unit tests need to pass before we can
release it. Who knows what else will fail in NSS or applications or libraries
which use NSS due to this.

As far as I can tell the manifestation of the problem is new. I'm guessing it's
showing up because the implementation of memcmp is different. I first saw it
when running NSS under a new version of valgrind, which in fact supplies it's
own version of memcmp.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch proposed by John Dennis

Comment 2

[Kai Engert (:KaiE:)]

(Please set keyword checkin-needed after we get an r+ )

Comment 3

[Wan-Teh Chang]

Comment on attachment 607526 [details] [diff] [review]
Patch proposed by John Dennis

r=wtc.  Patch checked in on the NSS trunk (NSS 3.13.4).

Checking in secitem.c;
/cvsroot/mozilla/security/nss/lib/util/secitem.c,v  <--  secitem.c
new revision: 1.17; previous revision: 1.16
done

Comment 4

[Robert Relyea]

Comment on attachment 607526 [details] [diff] [review]
Patch proposed by John Dennis

r+ rrelyea