Skip to content

Commit 80ff52a

Browse files
rmw42rmw42
committed
Code review changes
Rework SSH decoding and tests Fix encoding and tests COMPARE_TESTVECTOR macro Single return point in ssh_decode_sequence_multi Actually use XSTRNCPY rather than just defining it More code review fixes
1 parent 9c9c621 commit 80ff52a

File tree

9 files changed

+225
-169
lines changed

9 files changed

+225
-169
lines changed

‎doc/crypt.tex

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5254,17 +5254,6 @@ \subsection{Key Helper Functions}
52545254
\textit{out} is where the ASCII output is placed, and the \textit{outlen}
52555255
parameter is updated to hold the output OID size (including NUL byte).
52565256

5257-
To check that the curve OID matches a NUL--terminated string the following function is provided:
5258-
5259-
\index{ecc\_cmp\_oid\_str()}
5260-
\begin{verbatim}
5261-
int ecc_cmp_oid_str(const char *oidstr,
5262-
const ecc_key *key);
5263-
\end{verbatim}
5264-
5265-
Where \textit{key} is the key for which we want to check its OID, \textit{oidstr} is the OID string
5266-
we want to compare against. This returns \textit{CRYPT\_OK} if the curve has the desired OID.
5267-
52685257
\mysection{Key Export and Import}
52695258

52705259
\subsection{Export Raw Key}
@@ -7788,7 +7777,7 @@ \subsubsection{Endianness}
77887777
There are also options you can specify from the \textit{tomcrypt\_custom.h} header file.
77897778

77907779
\subsection{X memory routines}
7791-
\index{XMALLOC}\index{XREALLOC}\index{XCALLOC}\index{XFREE}\index{XMEMSET}\index{XMEMCPY}\index{XMEMMOVE}\index{XMEMCMP}\index{XSTRCMP}
7780+
\index{XMALLOC}\index{XREALLOC}\index{XCALLOC}\index{XFREE}\index{XMEMSET}\index{XMEMCPY}\index{XMEMMOVE}\index{XMEMCMP}\index{XSTRCMP}\index{XSTRNCPY}
77927781
At the top of tomcrypt\_custom.h are a series of macros denoted as XMALLOC, XCALLOC, XREALLOC, XFREE, and so on. They resolve to
77937782
the name of the respective functions from the standard C library by default. This lets you substitute in your own memory routines.
77947783
If you substitute in your own functions they must behave like the standard C library functions in terms of what they expect as input and

‎helper.pl

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ sub check_source {
5353
push @{$troubles->{unwanted_memmove}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bmemmove\s*\(/;
5454
push @{$troubles->{unwanted_memcmp}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bmemcmp\s*\(/;
5555
push @{$troubles->{unwanted_strcmp}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bstrcmp\s*\(/;
56+
push @{$troubles->{unwanted_strcpy}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bstrcpy\s*\(/;
57+
push @{$troubles->{unwanted_strncpy}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bstrncpy\s*\(/;
5658
push @{$troubles->{unwanted_clock}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bclock\s*\(/;
5759
push @{$troubles->{unwanted_qsort}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bqsort\s*\(/;
5860
push @{$troubles->{sizeof_no_brackets}}, $lineno if $file =~ /^src\/.*\.c$/ && $l =~ /\bsizeof\s*[^\(]/;

‎src/headers/tomcrypt_custom.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@
4343
#define XMEM_NEQ mem_neq
4444
#endif
4545
#ifndef XSTRCMP
46-
#define XSTRCMP strcmp
46+
#define XSTRCMP strcmp
47+
#endif
48+
#ifndef XSTRNCPY
49+
#define XSTRNCPY strncpy
4750
#endif
4851

4952
#ifndef XCLOCK
@@ -56,7 +59,7 @@
5659

5760
#if ( defined(malloc) || defined(realloc) || defined(calloc) || defined(free) || \
5861
defined(memset) || defined(memcpy) || defined(memcmp) || defined(strcmp) || \
59-
defined(clock) || defined(qsort) ) && !defined(LTC_NO_PROTOTYPES)
62+
defined(strncpy) || defined(clock) || defined(qsort) ) && !defined(LTC_NO_PROTOTYPES)
6063
#define LTC_NO_PROTOTYPES
6164
#endif
6265

‎src/misc/ssh/ssh_decode_sequence_multi.c

Lines changed: 44 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -18,115 +18,99 @@
1818

1919
/**
2020
Decode a SSH sequence using a VA list
21-
@param out [out] Destination for data
22-
@param outlen [in/out] Length of buffer and resulting length of output
21+
@param in Data to decode
22+
@param inlen Length of buffer to decode
2323
@remark <...> is of the form <type, data> (int, void*) except for string <type, data, size>
2424
@return CRYPT_OK on success
2525
*/
2626
int ssh_decode_sequence_multi(const unsigned char *in, unsigned long inlen, ...)
2727
{
28+
const unsigned char *sentinel = in + inlen;
2829
int err;
29-
ssh_data_type type;
30-
void *data;
3130
va_list args;
31+
ssh_data_type type;
32+
void *vdata;
33+
unsigned char *cdata;
34+
char *sdata;
35+
ulong32 *u32data;
36+
ulong64 *u64data;
3237
unsigned long size, bufsize;
3338

3439
LTC_ARGCHK(in != NULL);
3540

3641
/* Decode values from buffer */
3742
va_start(args, inlen);
38-
for (;;) {
39-
type = (ssh_data_type)va_arg(args, int);
40-
data = va_arg(args, void*);
41-
LTC_UNUSED_PARAM(data);
42-
43-
if (type == LTC_SSHDATA_EOL) {
44-
break;
45-
}
46-
43+
while ((type = (ssh_data_type)va_arg(args, int)) != LTC_SSHDATA_EOL) {
4744
switch (type) {
4845
case LTC_SSHDATA_BYTE:
49-
size = ((unsigned long)in[0]);
50-
in++;
51-
*(unsigned long *)data = size;
46+
cdata = va_arg(args, unsigned char*);
47+
*cdata = *in++;
5248
break;
5349
case LTC_SSHDATA_BOOLEAN:
54-
size = ((unsigned long)in[0]);
55-
in++;
50+
cdata = va_arg(args, unsigned char*);
5651
/*
5752
The value 0 represents FALSE, and the value 1 represents TRUE. All non-zero values MUST be
5853
interpreted as TRUE; however, applications MUST NOT store values other than 0 and 1.
5954
*/
60-
*(unsigned long *)data = (size)?1:0;
55+
*cdata = (*in++)?1:0;
6156
break;
6257
case LTC_SSHDATA_UINT32:
63-
size = ((unsigned long)in[0] << 24) |
64-
((unsigned long)in[1] << 16) |
65-
((unsigned long)in[2] << 8) |
66-
((unsigned long)in[3]);
58+
u32data = va_arg(args, ulong32*);
59+
LOAD32H(*u32data, in);
6760
in += 4;
68-
*(unsigned long *)data = size;
6961
break;
7062
case LTC_SSHDATA_UINT64:
71-
size = ((unsigned long)in[0] << 56) |
72-
((unsigned long)in[1] << 48) |
73-
((unsigned long)in[2] << 40) |
74-
((unsigned long)in[3] << 32) |
75-
((unsigned long)in[4] << 24) |
76-
((unsigned long)in[5] << 16) |
77-
((unsigned long)in[6] << 8) |
78-
((unsigned long)in[7]);
63+
u64data = va_arg(args, ulong64*);
64+
LOAD64H(*u64data, in);
7965
in += 8;
80-
*(unsigned long *)data = size;
8166
break;
8267
case LTC_SSHDATA_STRING:
8368
case LTC_SSHDATA_NAMELIST:
69+
sdata = va_arg(args, char*);
8470
bufsize = va_arg(args, unsigned long);
85-
size = ((unsigned long)in[0] << 24) |
86-
((unsigned long)in[1] << 16) |
87-
((unsigned long)in[2] << 8) |
88-
((unsigned long)in[3]);
71+
LOAD32H(size, in);
8972
in += 4;
9073
if (size > 0) {
9174
if (size >= bufsize) {
92-
va_end(args);
93-
return CRYPT_BUFFER_OVERFLOW;
75+
err = CRYPT_BUFFER_OVERFLOW;
76+
goto error;
9477
}
95-
strncpy((char *)data, (const char *)in, size);
96-
((char *)data)[size] = 0; /* strncpy doesn't null terminate */
78+
XSTRNCPY(sdata, (const char *)in, size);
79+
sdata[size] = '\0'; /* strncpy doesn't NUL-terminate */
9780
} else {
98-
*(char *)data = 0;
81+
*sdata = '\0';
9982
}
10083
in += size;
10184
break;
10285
case LTC_SSHDATA_MPINT:
103-
size = ((unsigned long)in[0] << 24) |
104-
((unsigned long)in[1] << 16) |
105-
((unsigned long)in[2] << 8) |
106-
((unsigned long)in[3]);
86+
vdata = va_arg(args, void*);
87+
LOAD32H(size, in);
10788
in += 4;
10889
if (size == 0) {
109-
if ((err = mp_set(data, 0)) != CRYPT_OK) {
110-
va_end(args);
111-
return CRYPT_ERROR;
112-
}
90+
if ((err = mp_set(vdata, 0)) != CRYPT_OK) { goto error; }
11391
} else {
114-
if ((err = mp_read_unsigned_bin(data, (unsigned char *)in, size)) != CRYPT_OK) {
115-
va_end(args);
116-
return CRYPT_ERROR;
117-
}
92+
if ((err = mp_read_unsigned_bin(vdata, (unsigned char *)in, size)) != CRYPT_OK) { goto error; }
11893
}
11994
in += size;
12095
break;
12196

122-
case LTC_SSHDATA_EOL: /* Should never get here */
123-
va_end(args);
124-
return CRYPT_INVALID_ARG;
125-
}
97+
case LTC_SSHDATA_EOL:
98+
/* Should never get here */
99+
err = CRYPT_INVALID_ARG;
100+
goto error;
101+
}
102+
103+
/* Check we've not read too far */
104+
if (in > sentinel) {
105+
err = CRYPT_BUFFER_OVERFLOW;
106+
goto error;
107+
}
126108
}
127-
va_end(args);
109+
err = CRYPT_OK;
128110

129-
return CRYPT_OK;
111+
error:
112+
va_end(args);
113+
return err;
130114
}
131115

132116
#endif

0 commit comments

Comments
 (0)