Fix SSL client connection crash for SAN marked critical The patch for CVE-2013-4073 (https://gist.github.com/nahi/5880963) caused SSL crash when a SSL server returns the certificate that has critical SAN value. X509 extension could include 2 or 3 elements in it; [id, criticality, octet_string] if critical, [id, octet_string] if not. Making sure to pick the last element of X509 extension and use it as SAN value.

  • Download Gist
0001-Fix-SSL-client-connection-crash-for-SAN-marked-criti.patch
Diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81
From 61c3537bd9f8e37b01a8e45644c489fd8696c94b Mon Sep 17 00:00:00 2001
From: Hiroshi Nakamura <nahi@ruby-lang.org>
Date: Fri, 5 Jul 2013 23:22:29 +0900
Subject: [PATCH] Fix SSL client connection crash for SAN marked critical
 
The patch for CVE-2013-4073 caused SSL crash when a SSL server returns
the certificate that has critical SAN value. X509 extension could
include 2 or 3 elements in it;
 
[id, criticality, octet_string] if critical,
[id, octet_string] if not.
 
Making sure to pick the last element of X509 extension and use it as SAN
value.
---
ext/openssl/lib/openssl/ssl.rb | 2 +-
test/openssl/test_ssl.rb | 27 +++++++++++++++++----------
2 files changed, 18 insertions(+), 11 deletions(-)
 
diff --git a/ext/openssl/lib/openssl/ssl.rb b/ext/openssl/lib/openssl/ssl.rb
index 04eb592..741274a 100644
--- a/ext/openssl/lib/openssl/ssl.rb
+++ b/ext/openssl/lib/openssl/ssl.rb
@@ -98,7 +98,7 @@ module OpenSSL
should_verify_common_name = true
cert.extensions.each{|ext|
next if ext.oid != "subjectAltName"
- id, ostr = OpenSSL::ASN1.decode(ext.to_der).value
+ ostr = OpenSSL::ASN1.decode(ext.to_der).value.last
sequence = OpenSSL::ASN1.decode(ostr.value)
sequence.value.each{|san|
case san.tag
diff --git a/test/openssl/test_ssl.rb b/test/openssl/test_ssl.rb
index daaaa0a..a3100a8 100644
--- a/test/openssl/test_ssl.rb
+++ b/test/openssl/test_ssl.rb
@@ -338,25 +338,32 @@ class OpenSSL::TestSSL < OpenSSL::SSLTestCase
end
def test_verify_certificate_identity
- # creating NULL byte SAN certificate
+ [true, false].each do |criticality|
+ cert = create_null_byte_SAN_certificate(criticality)
+ assert_equal(false, OpenSSL::SSL.verify_certificate_identity(cert, 'www.example.com'))
+ assert_equal(true, OpenSSL::SSL.verify_certificate_identity(cert, 'www.example.com\0.evil.com'))
+ assert_equal(false, OpenSSL::SSL.verify_certificate_identity(cert, '192.168.7.255'))
+ assert_equal(true, OpenSSL::SSL.verify_certificate_identity(cert, '192.168.7.1'))
+ assert_equal(false, OpenSSL::SSL.verify_certificate_identity(cert, '13::17'))
+ assert_equal(true, OpenSSL::SSL.verify_certificate_identity(cert, '13:0:0:0:0:0:0:17'))
+ end
+ end
+
+ # Create NULL byte SAN certificate
+ def create_null_byte_SAN_certificate(critical = false)
ef = OpenSSL::X509::ExtensionFactory.new
cert = OpenSSL::X509::Certificate.new
cert.subject = OpenSSL::X509::Name.parse "/DC=some/DC=site/CN=Some Site"
- ext = ef.create_ext('subjectAltName', 'DNS:placeholder,IP:192.168.7.1,IP:13::17')
+ ext = ef.create_ext('subjectAltName', 'DNS:placeholder,IP:192.168.7.1,IP:13::17', critical)
ext_asn1 = OpenSSL::ASN1.decode(ext.to_der)
san_list_der = ext_asn1.value.reduce(nil) { |memo,val| val.tag == 4 ? val.value : memo }
san_list_asn1 = OpenSSL::ASN1.decode(san_list_der)
san_list_asn1.value[0].value = 'www.example.com\0.evil.com'
- ext_asn1.value[1].value = san_list_asn1.to_der
+ pos = critical ? 2 : 1
+ ext_asn1.value[pos].value = san_list_asn1.to_der
real_ext = OpenSSL::X509::Extension.new ext_asn1
cert.add_extension(real_ext)
-
- assert_equal(false, OpenSSL::SSL.verify_certificate_identity(cert, 'www.example.com'))
- assert_equal(true, OpenSSL::SSL.verify_certificate_identity(cert, 'www.example.com\0.evil.com'))
- assert_equal(false, OpenSSL::SSL.verify_certificate_identity(cert, '192.168.7.255'))
- assert_equal(true, OpenSSL::SSL.verify_certificate_identity(cert, '192.168.7.1'))
- assert_equal(false, OpenSSL::SSL.verify_certificate_identity(cert, '13::17'))
- assert_equal(true, OpenSSL::SSL.verify_certificate_identity(cert, '13:0:0:0:0:0:0:17'))
+ cert
end
def test_tlsext_hostname
--
1.8.1.2

Using Array#last is very elegant. I came up with something much more complicated... Thanks, my friend, I'll commit the patch tonight! Do I need to inform someone when it's done, or will this simply be released with the next patch release?

Please sign in to comment on this gist.

Something went wrong with that request. Please try again.