[rs-dev] TransactionIDs on SCEP and filenames

[Dirk-Willem van Gulik]

After looking into this some more - it seems that below patch / more protection is actually needed - it prevents the 1 in 20 fails on a CISCO SCEP all (when there are things like dots and /-esh in the KeyID).

So at the least below safe_filename() is needed (and not just wise) for the client controled KeyID. Looking at the serial files - those should be safe as they are essentially numbers (assuming a sensible length).

Not yet committed.

With kind regards,

Dw

> On 7 Aug 2023, at 17:37, Dirk-Willem van Gulik <dirkx at webweaving.org> wrote:
> 
> It appears that we're sailing fairly close to the wind when using the transaction ID 'raw' as the start of the filename; relying on just
> apr_filepath_merge(... APR_FILEPATH_SECUREROOT | APR_FILEPATH_NOTRELATIVE ..) to trap shenanigans:
> 
> [Mon Aug 07 15:21:12.278390 2023] [ca_disk:error] [pid 15425] (20023)The given path was above the root path: [client 127.0.1.11:42999] 
> mod_ca_disk: The CADiskCertificateByTransactionPath could not be merged with: /tmp/ATNBEIl2XHzyFxwAp++Rv/uyBsQ=.cert
> 
> Would it make sense to simply SHA256 it; and use that instead as the file name (mod_ca_disk.c - near line 356).
> 
> It also prevents 'confusing' names. E.g. a transaction ID such as 'ca' or 'foo' will yield a file foo.cert or ca.cert 'near' the real ones.
> 
> Dw
> 

Index: mod_ca_disk.c
===================================================================
--- mod_ca_disk.c	(revision 432)
+++ mod_ca_disk.c	(working copy)
@@ -22,6 +22,7 @@
  */
 #include <apr_strings.h>
 #include <apr_hash.h>
+#include <apr_lib.h>
 
 #include <openssl/err.h>
 #include <openssl/evp.h>
@@ -294,6 +295,38 @@
     return n;
 }
 
+/* Prevent anything untowards in a filename; by replacing
+ * any non alpanums by a _<hex> construct.
+ */
+static const char * safe_filename(apr_pool_t *pool, const char * fname) 
+{
+	const char hex[16] = "0123456789ABCDEF";
+	char * out = NULL, * o;
+
+	if (fname == NULL)
+		return "_";
+
+	if (!*fname)
+		return "_00";
+
+	// worst case lenght; including trailing \0.
+	out = apr_palloc(pool, 1 + strlen(fname) * 3);
+
+        o = out;
+	for(const char *p = fname; *p; p++) 
+	{
+		if (apr_isalnum(*p)) 
+		{
+			*o++ = *p;
+			continue;
+		};
+		*o++ = '_';
+		*o++ = hex[ ( *p >> 4)  & 0xF ];
+		*o++ = hex[ ( *p >> 0)  & 0xF ];
+	};
+	return out;
+}
+
 static int ca_sign_disk(request_rec *r, apr_hash_t *params,
         const unsigned char **buffer, apr_size_t *len)
 {
@@ -377,7 +410,7 @@
 
         /* try write away the CSR as a PEM file */
         status = apr_filepath_merge(&fname, conf->csr_path,
-                apr_pstrcat(r->pool, transaction_id, ".csr", NULL),
+                apr_pstrcat(r->pool, safe_filename(r->pool, transaction_id), ".csr", NULL),
                 APR_FILEPATH_SECUREROOT | APR_FILEPATH_NOTRELATIVE, r->pool);
         if (APR_SUCCESS != status) {
             log_message(r, status, "The CSR path must be a valid path");
@@ -531,28 +564,27 @@
                         APR_HASH_KEY_STRING) :
                         NULL;
         if (transaction_id) {
+	    const char * kid;
             ASN1_STRING *s = parse_ASN1_STRING(r->pool, transaction_id);
-            if (s) {
+            if (!s) {
+                log_message(r, status, "The transactionID could not be parsed");
+
+                return HTTP_BAD_REQUEST;
+            }
+	    if (conf->serial_path)
+        	ap_log_rerror( APLOG_MARK, APLOG_WARNING, APR_SUCCESS, r, 
+			"Both serial and transaction path defined; using the latter.");
+
 #if HAVE_ASN1_STRING_GET0_DATA
-                key = apr_pstrcat(r->pool,
-                        apr_pstrndup(r->pool,
-                                (const char *) ASN1_STRING_get0_data(s),
-                                ASN1_STRING_length(s)), ".",
-                        conf->transaction_path_suffix, NULL);
+	    kid = (const char *) ASN1_STRING_get0_data(s);
 #else
-                key = apr_pstrcat(r->pool,
-                        apr_pstrndup(r->pool,
-                                (const char *) ASN1_STRING_data(s),
+            kid = (const char *) ASN1_STRING_data(s);
+#endif
+            key = apr_pstrcat(r->pool,
+                        apr_pstrndup(r->pool, safe_filename(r->pool,kid),
                                 ASN1_STRING_length(s)), ".",
                         conf->transaction_path_suffix, NULL);
-#endif
-            }
-            else {
-                log_message(r, status, "The transactionID could not be parsed");
 
-                return HTTP_BAD_REQUEST;
-            }
-
             /* set up the path */
             status = apr_filepath_merge(path ? &link : &path,
                     conf->transaction_path, key,
@@ -740,7 +772,7 @@
     }
 
     /* does the file exist? */
-    status = apr_filepath_merge(&fname, path, key,
+    status = apr_filepath_merge(&fname, path, safe_filename(r->pool, key),
             APR_FILEPATH_SECUREROOT | APR_FILEPATH_NOTRELATIVE, r->pool);
     if (APR_SUCCESS != status) {
         log_message(r, status, "The certificate was not found");