[go: nahoru, domu]

Skip to content

Commit

Permalink
CVE-2023-0286: Fix GENERAL_NAME_cmp for x400Address (1.1.1)
Browse files Browse the repository at this point in the history
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
  • Loading branch information
hlandau authored and levitte committed Feb 3, 2023
1 parent f040f25 commit 2c6c9d4
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 3 deletions.
18 changes: 17 additions & 1 deletion CHANGES
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,23 @@

Changes between 1.1.1s and 1.1.1t [xx XXX xxxx]

*)
*) Fixed a type confusion vulnerability relating to X.400 address processing
inside an X.509 GeneralName. X.400 addresses were parsed as an ASN1_STRING

This comment has been minimized.

Copy link
@Roo4L

Roo4L Feb 9, 2023

Hi! I am a maintainer from Cloudlinux, Inc. Could you help me please to identify whether my version of openssl is vulnerable or not.

As I see, CHANGES message tells: "X.400 addresses were parsed as an ASN1_STRING". Can you actually point where it happens exactly (function name(s)/code fragment)? I would like to see whether they are parsed like this in my version or not.

This comment has been minimized.

Copy link
@paulidale

paulidale Feb 9, 2023

Contributor

This commit shows the change. That's where it happens.

2c6c9d4#diff-c3891ec50d81d873dd6a01a5c67da03a5e137d4aea4d285284af43a3d04cb352

This comment has been minimized.

Copy link
@Roo4L

Roo4L Feb 10, 2023

As far, as I see, this code fragment only shows, that the x400Address were compared as ASN1_TYPE cmp function, but by this time they have been already parsed from source as ASN1_STRING somewhere else in the code. This "hidden" code is not touched by commit directly (because there is no need). I would like to find this code fragment responsible for parsing.

This comment has been minimized.

Copy link
@Roo4L

Roo4L Feb 10, 2023

Because if at some previous version this "hidden" code fragment was different somehow and parsed x400Address as ASN1_TYPE, it would mean that code is not affected at this previous version of openssl.

This comment has been minimized.

Copy link
@paulidale

paulidale Feb 10, 2023

Contributor

The problem is not in the parsing of the string, it's in the incorrect comparison being called on the results of the parse.

Did you read the advisory? All versions of OpenSSL from 1.0.2 are vulnerable. I would expect 1.1.0 and versions prior to 1.0.2 are also vulnerable, at least back as far as when this function was introduced. If you are running a version not mentioned in the advisory, you've got other unpatched CVEs to worry about.

This comment has been minimized.

Copy link
@Roo4L

Roo4L Feb 10, 2023

I understand that problem in not in parsing of the string, nevertheless I would like to see the code responsible of doing it. I am worried, because, for example, RedHat claims that their openssl on RHEL6 is not vulnerable, but source code still has GENERAL_NAME_cmp function with the same code fragment, but somehow redhat believes it is not vulnerable. I assume the have seen some sort of deference in parsing code and don't see any mismatch at their version of openssl. May be may guess is wrong, but still I am interested in verifying it.

This comment has been minimized.

Copy link
@Roo4L

Roo4L Feb 10, 2023

@paulidale Thanks for you replies! You ensured me that binary format should be ASN1_STRING and point about Security Advisory made me think that prolly RedHat stated that their version of openssl is not vulnerable due to info mentioned in advisory (openssl 1.0.1 is not mentioned at all) instead of verifying the code exactly. I'll contact them directly to get more information.

This comment has been minimized.

Copy link
@t8m

t8m Feb 11, 2023

Member

1.0.x versions are identical in this regard. However 0.9.8 does not seem to have GENERAL_NAME_cmp. I did not investigate in depth though.

This comment has been minimized.

Copy link
@davidben

davidben Feb 11, 2023

Contributor

I assume the have seen some sort of deference in parsing code and don't see any mismatch at their version of openssl.

The mismatch comes from crypto/asn1/tasn_*.c, which walk a type-erased table with offsetofs. The type in GENERAL_NAME's table did not match the C structure. OpenSSL's ASN.1 library is quite hairy and loses most type-checking in C, which is why the patch is odd.

1.0.x versions are identical in this regard. However 0.9.8 does not seem to have GENERAL_NAME_cmp. I did not investigate in depth though.

This was answered in the writeup I sent you all when I originally reported the issue. :-P

This bug has been present since OpenSSL 0.9.7, though there were no offending accesses within the library then.
The bug became reachable from X.509 CRL handling (and likely the Time-Stamp Protocol) in OpenSSL 1.0.0.

This comment has been minimized.

Copy link
@bangcheng

bangcheng Feb 13, 2023

Contributor

I found the ASN1 struct definition of x400Address in crypto\x509v3\v3_genn.c: ASN1_IMP(GENERAL_NAME, d.x400Address, ASN1_SEQUENCE, GEN_X400). X400Address is specified as the SEQUENCE type instead of ASN1_STRING. I'm curious how openssl converts this definition into ASN1_STRING.

but subsequently interpreted by GENERAL_NAME_cmp as an ASN1_TYPE. This
vulnerability may allow an attacker who can provide a certificate chain and
CRL (neither of which need have a valid signature) to pass arbitrary
pointers to a memcmp call, creating a possible read primitive, subject to
some constraints. Refer to the advisory for more information. Thanks to
David Benjamin for discovering this issue. (CVE-2023-0286)

This issue has been fixed by changing the public header file definition of
GENERAL_NAME so that x400Address reflects the implementation. It was not
possible for any existing application to successfully use the existing
definition; however, if any application references the x400Address field
(e.g. in dead code), note that the type of this field has changed. There is
no ABI change.

[Hugo Landau]

Changes between 1.1.1r and 1.1.1s [1 Nov 2022]

Expand Down
2 changes: 1 addition & 1 deletion crypto/x509v3/v3_genn.c
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ int GENERAL_NAME_cmp(GENERAL_NAME *a, GENERAL_NAME *b)
return -1;
switch (a->type) {
case GEN_X400:
result = ASN1_TYPE_cmp(a->d.x400Address, b->d.x400Address);
result = ASN1_STRING_cmp(a->d.x400Address, b->d.x400Address);
break;

case GEN_EDIPARTY:
Expand Down
2 changes: 1 addition & 1 deletion include/openssl/x509v3.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ typedef struct GENERAL_NAME_st {
OTHERNAME *otherName; /* otherName */
ASN1_IA5STRING *rfc822Name;
ASN1_IA5STRING *dNSName;
ASN1_TYPE *x400Address;
ASN1_STRING *x400Address;
X509_NAME *directoryName;
EDIPARTYNAME *ediPartyName;
ASN1_IA5STRING *uniformResourceIdentifier;
Expand Down
8 changes: 8 additions & 0 deletions test/v3nametest.c
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,14 @@ static struct gennamedata {
0xb7, 0x09, 0x02, 0x02
},
15
}, {
/*
* Regression test for CVE-2023-0286.
*/
{
0xa3, 0x00
},
2
}
};

Expand Down

0 comments on commit 2c6c9d4

Please sign in to comment.