x509_info.c containing sample/debug funcs

[gstrauss]

Description

x509_info.c containing sample/debug funcs Follows discussion in #5431.

• collect sample/debug x509 functions into x509_info.c
• replace mbedtls_snprintf with faster memcpy in most x509 info funcs
  • code size compiled -Os is slightly smaller, yet quite a bit faster
• remove use of MBEDTLS_X509_SAFE_SNPRINTF
  • macro is in a header, has no args, modifies assumed params, might return from function -- none of which is ideal for public macro

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

• [x] Tests
• [x] Changelog updated

[gstrauss]

As posted I posted in #5431: I expect that you will find it too large a change, but I wanted to share the code. lighttpd no longer uses these functions in common lighttpd code paths due to limitations in these mbedtls routines, such as not supporting non-ASCII UTF-8. (However, lighttpd mod_mbedtls does still use mbedtls_x509_crt_verify_info in an error condition with client certificate verification.)

Also from #5431:

Data point: I did a quick, non-scientific benchmark and found the mbedtls_x509_dn_gets() above compiled -O2 is approx 5x - 6x faster than the existing implementation in mbedtls library/x509.c.

I expect that other routines such as mbedtls_x509_crt_info() will be an even larger multiple speed increase due to replacing much more voluminous use of mbedtls_snprintf in that and other similar *_info routines.

[gstrauss]

rebased on 'development' and updated commit to add missing sign-off

As this is a large commit, I thought I might try to provide an intro: the debug/info functions are collected into new file library/x509_info.c. Most use of snprintf is replaced with more efficient helper functions which are local to library/x509_info.c. I would recommend starting the review by taking a look through the short helper functions starting around line 664 library/x509_info.c which avoid snprintf() by performing range checks and then using memcpy(), or else wrap snprintf().

• mbedtls_mempcpy_lim() copies bytes into a destination, but not beyond the provided limit. This is the central range check fn.

• mbedtls_allpcpy_lim() macro (line 67) uses sizeof(input)-1 on const string ("...") and then mbedtls_mempcpy_lim()

• mbedtls_strpcpy_lim() does strlen() on input and then mbedtls_mempcpy_lim()

• mbedtls_itoa_lim() stringifies a number, but not beyond the provided limit. (uses snprintf())

• mbedtls_x509_time_str() stringifies a date, but not beyond the provided limit. (uses snprintf())

• mbedtls_strterm() '\0'-terminates the string constructed using the above functions and returns the length of the string, but if the string was truncated, returns MBEDTLS_ERR_X509_BUFFER_TOO_SMALL. The string is still '\0'-terminated and valid, even if truncated (unless the destination size was 0).

[mpg]

Thanks! I'll try to take a look next week, though I'm not promising anything. IIUC, this is mostly about performance, so I think it has lesser priority than other issues. I'd still like us to merge those improvements at some point, but time is limited so we need to prioritize.

[gstrauss]

I concur. This is lower priority. My desire was to checkpoint while it was still fresh in my head. :)

[gstrauss]

Thanks! I'll try to take a look next week, though I'm not promising anything. IIUC, this is mostly about performance, so I think it has lesser priority than other issues. I'd still like us to merge those improvements at some point, but time is limited so we need to prioritize.

FYI: some of these funcs used in tests/, so the code could (slightly) speed up tests. Additionally, for #5620, I am adding additional usage of these routines to tests/

[gstrauss]

@wernerlewis I am guessing you were not aware of this PR from Feb.

I'll incorporate your recent changes to mbedtls_x509_dn_gets()

[gstrauss]

ping. branch has been rebased on HEAD of 'development'

Now that mbedtls 3.2.0 is out the door, I am hoping that #6003 "mbedtls_x509_time performance" and this might get on the schedule somewhere. Thanks.

[gstrauss]

ping. branch has been rebased on HEAD of 'development'

Now that mbedtls 3.2.0 is out the door, I am hoping that https://github.com/Mbed-TLS/mbedtls/pull/6003 "mbedtls_x509_time performance" and this might get on the schedule somewhere. Thanks.

While this is not a bug-fix, it does improve performance AND reduce code size.

[gstrauss]

I am going to remove the top commit so that binary compatibility is preserved for this PR.

commit 8e097c86ab965429bcf311263b705e6067b235f9 (HEAD -> x509_info_ttt)
Author: Glenn Strauss <[email protected]>
Date:   Wed Feb 2 11:40:38 2022 -0500

    Remove unused x509 helper func, macro
    
    Remove unused x509 helper func:  mbedtls_x509_key_size_helper()
    Remove unused x509 helper macro: MBEDTLS_X509_SAFE_SNPRINTF
    
    (Note: these were previously exposed in public mbedtls/x509.h)
    
    Signed-off-by: Glenn Strauss <[email protected]>

diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h
index 213efa04a..c7fcd2b49 100644
--- a/include/mbedtls/x509.h
+++ b/include/mbedtls/x509.h
@@ -358,7 +358,6 @@ int mbedtls_x509_sig_alg_gets( char *buf, size_t size, const mbedtls_x509_buf *s
                        mbedtls_pk_type_t pk_alg, mbedtls_md_type_t md_alg,
                        const void *sig_opts );
 #endif
-int mbedtls_x509_key_size_helper( char *buf, size_t buf_size, const char *name );
 int mbedtls_x509_string_to_names( mbedtls_asn1_named_data **head, const char *name );
 int mbedtls_x509_set_extension( mbedtls_asn1_named_data **head, const char *oid, size_t oid_len,
                         int critical, const unsigned char *val,
@@ -371,15 +370,6 @@ int mbedtls_x509_write_sig( unsigned char **p, unsigned char *start,
                     const char *oid, size_t oid_len,
                     unsigned char *sig, size_t size );
 
-#define MBEDTLS_X509_SAFE_SNPRINTF                          \
-    do {                                                    \
-        if( ret < 0 || (size_t) ret >= n )                  \
-            return( MBEDTLS_ERR_X509_BUFFER_TOO_SMALL );    \
-                                                            \
-        n -= (size_t) ret;                                  \
-        p += (size_t) ret;                                  \
-    } while( 0 )
-
 #ifdef __cplusplus
 }
 #endif
diff --git a/library/x509_info.c b/library/x509_info.c
index 53e823b73..9fb90cb7a 100644
--- a/library/x509_info.c
+++ b/library/x509_info.c
@@ -901,28 +901,4 @@ int mbedtls_x509_serial_gets( char *buf, size_t size, const mbedtls_x509_buf *se
     }
 }
 
-#if defined(MBEDTLS_X509_REMOVE_INFO)
-#include <stdio.h>
-#endif
-/*
- * Helper for writing "RSA key size", "EC key size", etc
- */
-int mbedtls_x509_key_size_helper( char *buf, size_t buf_size, const char *name )
-{
-    /* XXX: unused func; deprecate */
-  #if 0 /*(limit use of mbedtls_*pcpy_lim() to !MBEDTLS_X509_REMOVE_INFO)*/
-    char *p = buf;
-    const char * const lim = buf + buf_size;
-    p = mbedtls_strpcpy_lim( p, lim, name );
-    p = mbedtls_allpcpy_lim( p, lim, " key size" );
-    int ret = (int)mbedtls_strterm( buf, p, lim );
-    return ( ( ret >= 0 ) ? 0 : ret );
-  #else
-    int ret = mbedtls_snprintf(buf, buf_size, "%s key size", name);
-    if( ret < 0 || (size_t) ret >= buf_size )
-        return( MBEDTLS_ERR_X509_BUFFER_TOO_SMALL );
-    return( 0 );
-  #endif
-}
-
 #endif /* MBEDTLS_X509_USE_C */