Skip to content

Commit 31458a1

Browse files
committed
Only return VERIFY_FAILED from a single point
Everything else is a fatal error. Also improve documentation about that for the vrfy callback.
1 parent d15795a commit 31458a1

File tree

8 files changed

+22
-6
lines changed

8 files changed

+22
-6
lines changed

Diff for: ChangeLog

+3
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ Changes
66
* Certificate verification functions now set flags to -1 in case the full
77
chain was not verified due to an internal error (including in the verify
88
callback) or chain length limitations.
9+
* With authmode set to optional, handshake is now aborted if the
10+
verification of the peer's certificate failed due to an overlong chain or
11+
a fatal error in the vrfy callback.
912

1013
= mbed TLS 2.5.1 released 2017-06-21
1114

Diff for: include/mbedtls/error.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@
7171
* Name ID Nr of Errors
7272
* PEM 1 9
7373
* PKCS#12 1 4 (Started from top)
74-
* X509 2 19
74+
* X509 2 20
7575
* PKCS5 2 4 (Started from top)
7676
* DHM 3 9
7777
* PK 3 14 (Started from top)

Diff for: include/mbedtls/ssl.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -1052,7 +1052,7 @@ void mbedtls_ssl_conf_authmode( mbedtls_ssl_config *conf, int authmode );
10521052
*
10531053
* If set, the verify callback is called for each
10541054
* certificate in the chain. For implementation
1055-
* information, please see \c x509parse_verify()
1055+
* information, please see \c mbedtls_x509_crt_verify()
10561056
*
10571057
* \param conf SSL configuration
10581058
* \param f_vrfy verification function

Diff for: include/mbedtls/x509.h

+1
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@
7676
#define MBEDTLS_ERR_X509_ALLOC_FAILED -0x2880 /**< Allocation of memory failed. */
7777
#define MBEDTLS_ERR_X509_FILE_IO_ERROR -0x2900 /**< Read/write of file failed. */
7878
#define MBEDTLS_ERR_X509_BUFFER_TOO_SMALL -0x2980 /**< Destination buffer is too small. */
79+
#define MBEDTLS_ERR_X509_FATAL_ERROR -0x3000 /**< A fatal error occured, eg the chain is too long or the vrfy callback failed. */
7980
/* \} name */
8081

8182
/**

Diff for: include/mbedtls/x509_crt.h

+7-1
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,13 @@ int mbedtls_x509_crt_verify_info( char *buf, size_t size, const char *prefix,
267267
*
268268
* All flags left after returning from the callback
269269
* are also returned to the application. The function should
270-
* return 0 for anything but a fatal error.
270+
* return 0 for anything (including invalid certificates)
271+
* other than fatal error, as a non-zero return code
272+
* immediately aborts the verification process. For fatal
273+
* errors, a specific error code should be used (different
274+
* from MBEDTLS_ERR_X509_CERT_VERIFY_FAILED which should not
275+
* be returned at this point), or MBEDTLS_ERR_X509_FATAL_ERROR
276+
* can be used if no better code is available.
271277
*
272278
* \note In case verification failed, the results can be displayed
273279
* using \c mbedtls_x509_crt_verify_info()

Diff for: library/error.c

+2
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,8 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen )
480480
mbedtls_snprintf( buf, buflen, "X509 - Read/write of file failed" );
481481
if( use_ret == -(MBEDTLS_ERR_X509_BUFFER_TOO_SMALL) )
482482
mbedtls_snprintf( buf, buflen, "X509 - Destination buffer is too small" );
483+
if( use_ret == -(MBEDTLS_ERR_X509_FATAL_ERROR) )
484+
mbedtls_snprintf( buf, buflen, "X509 - A fatal error occured, eg the chain is too long or the vrfy callback failed" );
483485
#endif /* MBEDTLS_X509_USE_C || MBEDTLS_X509_CREATE_C */
484486
// END generated code
485487

Diff for: library/x509_crt.c

+6-2
Original file line numberDiff line numberDiff line change
@@ -2057,8 +2057,8 @@ static int x509_crt_verify_child(
20572057
/* path_cnt is 0 for the first intermediate CA */
20582058
if( 1 + path_cnt > MBEDTLS_X509_MAX_INTERMEDIATE_CA )
20592059
{
2060-
*flags |= MBEDTLS_X509_BADCERT_NOT_TRUSTED;
2061-
return( MBEDTLS_ERR_X509_CERT_VERIFY_FAILED );
2060+
/* return immediately as the goal is to avoid unbounded recursion */
2061+
return( MBEDTLS_ERR_X509_FATAL_ERROR );
20622062
}
20632063

20642064
if( mbedtls_x509_time_is_past( &child->valid_to ) )
@@ -2310,6 +2310,10 @@ int mbedtls_x509_crt_verify_with_profile( mbedtls_x509_crt *crt,
23102310
}
23112311

23122312
exit:
2313+
/* prevent misuse of the vrfy callback */
2314+
if( ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED )
2315+
ret = MBEDTLS_ERR_X509_FATAL_ERROR;
2316+
23132317
if( ret != 0 )
23142318
{
23152319
*flags = (uint32_t) -1;

Diff for: tests/suites/test_suite_x509parse.data

+1-1
Original file line numberDiff line numberDiff line change
@@ -1204,7 +1204,7 @@ mbedtls_x509_crt_verify_max:"data_files/test-ca2.crt":"data_files/dir-maxpath":M
12041204

12051205
X509 CRT verify long chain (max intermediate CA + 1)
12061206
depends_on:MBEDTLS_SHA256_C:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED
1207-
mbedtls_x509_crt_verify_max:"data_files/dir-maxpath/00.crt":"data_files/dir-maxpath":MBEDTLS_X509_MAX_INTERMEDIATE_CA+1:MBEDTLS_ERR_X509_CERT_VERIFY_FAILED:-1
1207+
mbedtls_x509_crt_verify_max:"data_files/dir-maxpath/00.crt":"data_files/dir-maxpath":MBEDTLS_X509_MAX_INTERMEDIATE_CA+1:MBEDTLS_ERR_X509_FATAL_ERROR:-1
12081208

12091209
X509 CRT verify chain #1 (zero pathlen intermediate)
12101210
depends_on:MBEDTLS_SHA256_C:MBEDTLS_RSA_C

0 commit comments

Comments
 (0)