Fix SSH API

doc/crypt.tex

@@ -7233,6 +7233,73 @@ \subsection{HKDF Extract-and-Expand}
 
 Parameters are as in \textit{hkdf\_extract()} and \textit{hkdf\_expand()}.
 
+
+\mysection{SSH}
+
+The library provides functions to encode and decode SSH data as specified in RFC4251 Ch. 5.
+
+\subsection{Data types}
+
+The following enum is used to indicate a specific SSH data type
+(besides EOL which is an internal one that indicates the end of a sequence).
+
+\begin{figure}[h]
+\begin{center}
+\begin{small}
+\begin{tabular}{|l|l|l|}
+\hline \textbf{Definition}    & \textbf{arg data Type} & \textbf{SSH Type} \\
+\hline LTC\_SSHDATA\_EOL      & -                      & End of SSH data sequence. \\
+\hline LTC\_SSHDATA\_BYTE     & \texttt{unsigned char} & \texttt{byte} type \\
+\hline LTC\_SSHDATA\_BOOLEAN  & \texttt{unsigned char} & \texttt{boolean} type \\
+\hline LTC\_SSHDATA\_UINT32   & \texttt{ulong32}       & \texttt{uint32} \\
+\hline LTC\_SSHDATA\_UINT64   & \texttt{ulong64}       & \texttt{uint64} \\
+\hline LTC\_SSHDATA\_STRING   & \texttt{char*}         & \texttt{string} (one octet per char) \\
+\hline LTC\_SSHDATA\_MPINT    & \texttt{mp\_int}       & \texttt{mpint} \\
+\hline LTC\_SSHDATA\_NAMELIST & \texttt{char*}         & \texttt{name-list} (which works exactly like a \texttt{string}) \\
+\hline
+\end{tabular}
+\caption{List of SSH Supported Types}
+\index{ssh\_data\_type}
+\end{small}
+\end{center}
+\end{figure}
+
+\subsection{De- and Encoding with Multiple Argument Lists}
+
+\index{ssh\_encode\_sequence\_multi()}
+\index{ssh\_decode\_sequence\_multi()}
+
+
+The API works similar to the ASN.1 SEQUENCE multi en- and decoders.
+
+They either encode or decode a sequence of the supported SSH types where the items are specified after the length parameter.
+
+
+\begin{verbatim}
+int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);
+\end{verbatim}
+
+Where \texttt{out} points to the destination buffer and  \texttt{outlen} points 
+on function invocation to the length of the destination buffer 
+and after returning it will be filled with the number of octets written to the buffer.
+
+The encoding function \texttt{ssh\_encode\_sequence\_multi()} expects its items to be a pair of \texttt{(type, data)},
+except for the \texttt{string} resp. \texttt{name-list} type, which expects the triple \texttt{(type, data, size)}
+with \texttt{size} being of type \texttt{unsigned long}.
+
+
+\begin{verbatim}
+int ssh_decode_sequence_multi(const unsigned char *in, unsigned long *inlen, ...);
+\end{verbatim}
+
+Where \texttt{in} points to the buffer with the sequence to decode and \texttt{inlen} points 
+on function invocation to the length of the sequence
+and after returning it will be filled with the decoded number of octets.
+
+The decoding function \texttt{ssh\_decode\_sequence\_multi()} expects its items to be a pair of \texttt{(type, data*)},
+except for the \texttt{string} resp. \texttt{name-list} type, which expects the triple \texttt{(type, data, size*)}
+with \texttt{size*} being of type \texttt{unsigned long*}.
+
 \chapter{Miscellaneous}
 \mysection{Base64 Encoding and Decoding}
 The library provides functions to encode and decode a RFC 4648 Base64 coding scheme.

src/headers/tomcrypt_misc.h

@@ -163,19 +163,19 @@ int padding_depad(const unsigned char *data, unsigned long *length, unsigned lon
 
 #ifdef LTC_SSH
 typedef enum ssh_data_type_ {
+   LTC_SSHDATA_EOL,
    LTC_SSHDATA_BYTE,
    LTC_SSHDATA_BOOLEAN,
    LTC_SSHDATA_UINT32,
    LTC_SSHDATA_UINT64,
    LTC_SSHDATA_STRING,
    LTC_SSHDATA_MPINT,
    LTC_SSHDATA_NAMELIST,
-   LTC_SSHDATA_EOL
 } ssh_data_type;
 
 /* VA list handy helpers with tuples of <type, data> */
 int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...);
-int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...);
+int ssh_decode_sequence_multi(const unsigned char *in, unsigned long *inlen, ...);
 #endif /* LTC_SSH */
 
 int compare_testvector(const void* is, const unsigned long is_len, const void* should, const unsigned long should_len, const char* what, int which);

src/misc/ssh/ssh_decode_sequence_multi.c

@@ -18,12 +18,12 @@
 
 /**
   Decode a SSH sequence using a VA list
-  @param in     Data to decode
-  @param inlen  Length of buffer to decode
-  @remark <...> is of the form <type, data> (int, void*) except for string <type, data, size>
+  @param in     The input buffer
+  @param inlen  [in/out] The length of the input buffer and on output the amount of decoded data
+  @remark <...> is of the form <type, data*> (int, <unsigned char*,ulong32*,ulong64*>) except for string&name-list <type, data, size*> (int, void*, unsigned long*)
   @return CRYPT_OK on success
 */
-int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
+int ssh_decode_sequence_multi(const unsigned char *in, unsigned long *inlen, ...)
 {
    int           err;
    va_list       args;
@@ -33,11 +33,14 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
    char          *sdata;
    ulong32       *u32data;
    ulong64       *u64data;
-   unsigned long bufsize;
+   unsigned long *bufsize;
    ulong32       size;
+   unsigned long remaining;
 
    LTC_ARGCHK(in    != NULL);
+   LTC_ARGCHK(inlen != NULL);
 
+   remaining = *inlen;
    /* Decode values from buffer */
    va_start(args, inlen);
    while ((type = (ssh_data_type)va_arg(args, int)) != LTC_SSHDATA_EOL) {
@@ -47,7 +50,7 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
           type == LTC_SSHDATA_MPINT)
       {
          /* Check we'll not read too far */
-         if (inlen < 4) {
+         if (remaining < 4) {
             err = CRYPT_BUFFER_OVERFLOW;
             goto error;
          }
@@ -71,7 +74,7 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
          case LTC_SSHDATA_MPINT:
             LOAD32H(size, in);
             in += 4;
-            inlen -= 4;
+            remaining -= 4;
             break;
 
          case LTC_SSHDATA_EOL:
@@ -81,55 +84,63 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
       }
 
       /* Check we'll not read too far */
-      if (inlen < size) {
+      if (remaining < size) {
          err = CRYPT_BUFFER_OVERFLOW;
          goto error;
       } else {
-         inlen -= size;
+         remaining -= size;
+      }
+
+      vdata = va_arg(args, void*);
+      if (vdata == NULL) {
+         err = CRYPT_INVALID_ARG;
+         goto error;
       }
 
       /* Read data */
       switch (type) {
          case LTC_SSHDATA_BYTE:
-            cdata = va_arg(args, unsigned char*);
+            cdata = vdata;
             *cdata = *in++;
             break;
          case LTC_SSHDATA_BOOLEAN:
-            cdata = va_arg(args, unsigned char*);
+            cdata = vdata;
             /*
                The value 0 represents FALSE, and the value 1 represents TRUE.  All non-zero values MUST be
                interpreted as TRUE; however, applications MUST NOT store values other than 0 and 1.
-            */
+             */
             *cdata = (*in++)?1:0;
             break;
          case LTC_SSHDATA_UINT32:
-            u32data = va_arg(args, ulong32*);
+            u32data = vdata;
             LOAD32H(*u32data, in);
             in += 4;
             break;
          case LTC_SSHDATA_UINT64:
-            u64data = va_arg(args, ulong64*);
+            u64data = vdata;
             LOAD64H(*u64data, in);
             in += 8;
             break;
          case LTC_SSHDATA_STRING:
          case LTC_SSHDATA_NAMELIST:
-            sdata = va_arg(args, char*);
-            bufsize = va_arg(args, unsigned long);
-            if (size >= bufsize) {
+            sdata = vdata;
+            bufsize = va_arg(args, unsigned long*);
+            if (bufsize == NULL) {
+               err = CRYPT_INVALID_ARG;
+               goto error;
+            }
+            if (size + 1 >= *bufsize) {
                err = CRYPT_BUFFER_OVERFLOW;
                goto error;
             }
             if (size > 0) {
-               XSTRNCPY(sdata, (const char *)in, size);
-               sdata[size] = '\0'; /* strncpy doesn't NUL-terminate */
-            } else {
-               *sdata = '\0';
+               XMEMCPY(sdata, (const char *)in, size);
             }
+            sdata[size] = '\0';
+            *bufsize = size;
             in += size;
             break;
          case LTC_SSHDATA_MPINT:
-            vdata = va_arg(args, void*);
             if (size == 0) {
                if ((err = mp_set(vdata, 0)) != CRYPT_OK)                                                { goto error; }
             } else if ((in[0] & 0x80) != 0) {
@@ -150,6 +161,8 @@ int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
    }
    err = CRYPT_OK;
 
+   *inlen -= remaining;
+
 error:
    va_end(args);
    return err;

src/misc/ssh/ssh_encode_sequence_multi.c

@@ -20,7 +20,7 @@
   Encode a SSH sequence using a VA list
   @param out    [out] Destination for data
   @param outlen [in/out] Length of buffer and resulting length of output
-  @remark <...> is of the form <type, data> (int, void*)
+  @remark <...> is of the form <type, data> (int, <int,ulong32,ulong64>) except for string&name-list <type, data, size> (int, void*, unsigned long)
   @return CRYPT_OK on success
 */
 int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...)
@@ -29,8 +29,8 @@ int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...)
    va_list       args;
    ulong32       size;
    ssh_data_type type;
-   void         *vdata;
-   const char   *sdata;
+   void          *vdata;
+   const char    *sdata;
    int           idata;
    ulong32       u32data;
    ulong64       u64data;
@@ -58,9 +58,9 @@ int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...)
             break;
          case LTC_SSHDATA_STRING:
          case LTC_SSHDATA_NAMELIST:
-            sdata = va_arg(args, char*);
+            LTC_UNUSED_PARAM( va_arg(args, char*) );
+            size += va_arg(args, unsigned long);
             size += 4;
-            size += strlen(sdata);
             break;
          case LTC_SSHDATA_MPINT:
             vdata = va_arg(args, void*);
@@ -102,7 +102,7 @@ int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...)
             /*
                The value 0 represents FALSE, and the value 1 represents TRUE.  All non-zero values MUST be
                interpreted as TRUE; however, applications MUST NOT store values other than 0 and 1.
-            */
+             */
             *out++ = (idata)?1:0;
             break;
          case LTC_SSHDATA_UINT32:
@@ -118,7 +118,7 @@ int ssh_encode_sequence_multi(unsigned char *out, unsigned long *outlen, ...)
          case LTC_SSHDATA_STRING:
          case LTC_SSHDATA_NAMELIST:
             sdata = va_arg(args, char*);
-            size = strlen(sdata);
+            size = va_arg(args, unsigned long);
             STORE32H(size, out);
             out += 4;
             XMEMCPY(out, sdata, size);

src/pk/ecc/ecc_recover_key.c

@@ -114,19 +114,20 @@ int ecc_recover_key(const unsigned char *sig,  unsigned long siglen,
 #ifdef LTC_SSH
    else if (sigformat == LTC_ECCSIG_RFC5656) {
       char name[64], name2[64];
-      unsigned long namelen = sizeof(name2);
+      unsigned long namelen = sizeof(name);
+      unsigned long name2len = sizeof(name2);
 
       /* Decode as SSH data sequence, per RFC4251 */
-      if ((err = ssh_decode_sequence_multi(sig, siglen,
-                                           LTC_SSHDATA_STRING, name, 64,
+      if ((err = ssh_decode_sequence_multi(sig, &siglen,
+                                           LTC_SSHDATA_STRING, name, &namelen,
                                            LTC_SSHDATA_MPINT,  r,
                                            LTC_SSHDATA_MPINT,  s,
                                            LTC_SSHDATA_EOL,    NULL)) != CRYPT_OK)                      { goto error; }
 
 
       /* Check curve matches identifier string */
-      if ((err = ecc_ssh_ecdsa_encode_name(name2, &namelen, key)) != CRYPT_OK)                                { goto error; }
-      if (XSTRCMP(name,name2) != 0) {
+      if ((err = ecc_ssh_ecdsa_encode_name(name2, &name2len, key)) != CRYPT_OK)                                { goto error; }
+      if ((namelen != name2len) || (XSTRCMP(name, name2) != 0)) {
          err = CRYPT_INVALID_ARG;
          goto error;
       }

src/pk/ecc/ecc_sign_hash.c

@@ -164,7 +164,7 @@ int ecc_sign_hash_ex(const unsigned char *in,  unsigned long inlen,
 
       /* Store as SSH data sequence, per RFC4251 */
       err = ssh_encode_sequence_multi(out, outlen,
-                                      LTC_SSHDATA_STRING, name,
+                                      LTC_SSHDATA_STRING, name, namelen,
                                       LTC_SSHDATA_MPINT,  r,
                                       LTC_SSHDATA_MPINT,  s,
                                       LTC_SSHDATA_EOL,    NULL);

src/pk/ecc/ecc_ssh_ecdsa_encode_name.c

@@ -25,8 +25,7 @@ int ecc_ssh_ecdsa_encode_name(char *buffer, unsigned long *buflen, const ecc_key
 {
    char oidstr[64];
    unsigned long oidlen = sizeof(oidstr);
-   unsigned long size = 0;
-   int err;
+   int err, size = 0;
 
    LTC_ARGCHK(buffer != NULL);
    LTC_ARGCHK(buflen != NULL);
@@ -52,8 +51,11 @@ int ecc_ssh_ecdsa_encode_name(char *buffer, unsigned long *buflen, const ecc_key
       size = snprintf(buffer, *buflen, "ecdsa-sha2-%s", oidstr);
    }
 
-   /* snprintf returns size that would have been written, but limits to buflen-1 chars plus terminator */
-   if (size >= *buflen) {
+   /* snprintf returns a negative value on error
+    * or the size that would have been written, but limits to buflen-1 chars plus terminator */
+   if (size < 0) {
+      err = CRYPT_ERROR;
+   } else if ((unsigned)size >= *buflen) {
       err = CRYPT_BUFFER_OVERFLOW;
    } else {
       err = CRYPT_OK;

src/pk/ecc/ecc_verify_hash.c

@@ -100,19 +100,20 @@ int ecc_verify_hash_ex(const unsigned char *sig,  unsigned long siglen,
 #ifdef LTC_SSH
    else if (sigformat == LTC_ECCSIG_RFC5656) {
       char name[64], name2[64];
-      unsigned long namelen = sizeof(name2);
+      unsigned long namelen = sizeof(name);
+      unsigned long name2len = sizeof(name2);
 
       /* Decode as SSH data sequence, per RFC4251 */
-      if ((err = ssh_decode_sequence_multi(sig, siglen,
-                                           LTC_SSHDATA_STRING, name, 64,
+      if ((err = ssh_decode_sequence_multi(sig, &siglen,
+                                           LTC_SSHDATA_STRING, name, &namelen,
                                            LTC_SSHDATA_MPINT,  r,
                                            LTC_SSHDATA_MPINT,  s,
                                            LTC_SSHDATA_EOL,    NULL)) != CRYPT_OK)                      { goto error; }
 
 
       /* Check curve matches identifier string */
-      if ((err = ecc_ssh_ecdsa_encode_name(name2, &namelen, key)) != CRYPT_OK)                                { goto error; }
-      if (XSTRCMP(name,name2) != 0) {
+      if ((err = ecc_ssh_ecdsa_encode_name(name2, &name2len, key)) != CRYPT_OK)                                { goto error; }
+      if ((namelen != name2len) || (XSTRCMP(name, name2) != 0)) {
          err = CRYPT_INVALID_ARG;
          goto error;
       }

tests/common.h

@@ -17,10 +17,12 @@ extern prng_state yarrow_prng;
 #define DO(x) do { fprintf(stderr, "%s:\n", #x); run_cmd((x), __LINE__, __FILE__, #x, NULL); } while (0)
 #define DOX(x, str) do { fprintf(stderr, "%s - %s:\n", #x, (str)); run_cmd((x), __LINE__, __FILE__, #x, (str)); } while (0)
 #define SHOULD_FAIL(x) do { fprintf(stderr, "%s:\n", #x); run_cmd((x) != CRYPT_OK ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0)
+#define ENSURE(x) do { fprintf(stderr, "%s:\n", #x); run_cmd(((x)) ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0)
 #else
 #define DO(x) do { run_cmd((x), __LINE__, __FILE__, #x, NULL); } while (0)
 #define DOX(x, str) do { run_cmd((x), __LINE__, __FILE__, #x, (str)); } while (0)
 #define SHOULD_FAIL(x) do { run_cmd((x) != CRYPT_OK ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0)
+#define ENSURE(x) do { run_cmd(((x)) ? CRYPT_OK : CRYPT_FAIL_TESTVECTOR, __LINE__, __FILE__, #x, NULL); } while (0)
 #endif
 
 #define COMPARE_TESTVECTOR(i, il, s, sl, wa, wi) do { DO(do_compare_testvector((i), (il), (s), (sl), (wa), (wi))); } while(0)