Permalink
Browse files

Implemented server-side bounds checking on inccoming SPA data.

Enhanced the libfko decoding routine to include bounds checking on decrypted
SPA data.  This includes verifying the number of fields within incoming SPA
data (colon separated) along with verifying string lengths of each field.
  • Loading branch information...
mrash committed Jul 20, 2012
1 parent 8f500fd commit 4c7923413ed2f327ebc4875dcde98a04865e80d9
Showing with 78 additions and 11 deletions.
  1. +71 −10 lib/fko_decode.c
  2. +2 −1 lib/fko_encryption.c
  3. +5 −0 lib/fko_limits.h
View
@@ -5,7 +5,7 @@
*
* Author: Damien S. Stuart
*
- * Purpose: Decrypt and decode an FKO SPA message.
+ * Purpose: Decode an FKO SPA message after decryption.
*
* Copyright 2009-2010 Damien Stuart (dstuart@dstuart.org)
*
@@ -34,27 +34,35 @@
#include "base64.h"
#include "digest.h"
-/* Decrypt the encoded SPA data.
+/* Decode the encoded SPA data.
*/
int
fko_decode_spa_data(fko_ctx_t ctx)
{
- char *tbuf, *ndx;
- int t_size;
+ char *tbuf, *ndx, *tmp;
+ int t_size, i;
/* Check for required data.
*/
if(ctx->encoded_msg == NULL
|| strlen(ctx->encoded_msg) < MIN_SPA_ENCODED_MSG_SIZE)
return(FKO_ERROR_INVALID_DATA);
- /* Move the Digest to its place in the context.
+ /* Make sure there are enough fields in the SPA packet
+ * delimited with ':' chars
*/
- ndx = strrchr(ctx->encoded_msg, ':'); /* Find the last : in the data */
- if(ndx == NULL)
- return(FKO_ERROR_INVALID_DATA);
+ ndx = ctx->encoded_msg;
+ for (i=0; i < MAX_SPA_FIELDS; i++)
+ {
+ if ((tmp = strchr(ndx, ':')) == NULL)
+ break;
- ndx++;
+ ndx = tmp;
+ ndx++;
+ }
+
+ if (i < MIN_SPA_FIELDS)
+ return(FKO_ERROR_INVALID_DATA);
t_size = strlen(ndx);
@@ -132,7 +140,7 @@ fko_decode_spa_data(fko_ctx_t ctx)
/* We give up here if the computed digest does not match the
* digest in the message data.
*/
- if(strcmp(ctx->digest, tbuf))
+ if(strncmp(ctx->digest, tbuf, t_size))
{
free(tbuf);
return(FKO_ERROR_DIGEST_VERIFICATION_FAILED);
@@ -168,6 +176,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_USERNAME_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
strlcpy(tbuf, ndx, t_size+1);
ctx->username = malloc(t_size+1); /* Yes, more than we need */
@@ -188,6 +202,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_TIMESTAMP_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
strlcpy(tbuf, ndx, t_size+1);
ctx->timestamp = (unsigned int)atoi(tbuf);
@@ -201,6 +221,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_VERSION_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
ctx->version = malloc(t_size+1);
if(ctx->version == NULL)
{
@@ -219,6 +245,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_MESSAGE_TYPE_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
strlcpy(tbuf, ndx, t_size+1);
ctx->message_type = (unsigned int)atoi(tbuf);
@@ -232,6 +264,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_MESSAGE_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
strlcpy(tbuf, ndx, t_size+1);
ctx->message = malloc(t_size+1); /* Yes, more than we need */
@@ -257,6 +295,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_MESSAGE_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
strlcpy(tbuf, ndx, t_size+1);
ctx->nat_access = malloc(t_size+1); /* Yes, more than we need */
@@ -274,6 +318,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
ndx += t_size + 1;
if((t_size = strlen(ndx)) > 0)
{
+ if (t_size > MAX_SPA_MESSAGE_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
/* There is data, but what is it?
* If the message_type does not have a timeout, assume it is a
* server_auth field.
@@ -314,6 +364,12 @@ fko_decode_spa_data(fko_ctx_t ctx)
{
t_size = strcspn(ndx, ":");
+ if (t_size > MAX_SPA_MESSAGE_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
+
/* Looks like we have both, so assume this is the
*/
strlcpy(tbuf, ndx, t_size+1);
@@ -341,6 +397,11 @@ fko_decode_spa_data(fko_ctx_t ctx)
free(tbuf);
return(FKO_ERROR_INVALID_DATA);
}
+ if (t_size > MAX_SPA_MESSAGE_SIZE)
+ {
+ free(tbuf);
+ return(FKO_ERROR_INVALID_DATA);
+ }
/* Should be a number only.
*/
View
@@ -157,7 +157,8 @@ _rijndael_decrypt(fko_ctx_t ctx, const char *dec_key)
/* At this point we can check the data to see if we have a good
* decryption by ensuring the first field (16-digit random decimal
- * value) is valid and is followed by a colon.
+ * value) is valid and is followed by a colon. Additional checks
+ * are made in fko_decode_spa_data().
*/
ndx = (unsigned char *)ctx->encoded_msg;
for(i=0; i<FKO_RAND_VAL_SIZE; i++)
View
@@ -39,9 +39,14 @@
#define MAX_SPA_MESSAGE_SIZE 256
#define MAX_SPA_NAT_ACCESS_SIZE 128
#define MAX_SPA_SERVER_AUTH_SIZE 64
+#define MAX_SPA_TIMESTAMP_SIZE 12
+#define MAX_SPA_VERSION_SIZE 8 /* 12.34.56 */
+#define MAX_SPA_MESSAGE_TYPE_SIZE 2
#define MIN_SPA_ENCODED_MSG_SIZE 36 /* Somewhat arbitrary */
#define MIN_GNUPG_MSG_SIZE 400
+#define MIN_SPA_FIELDS 6
+#define MAX_SPA_FIELDS 10
/* Misc.
*/

0 comments on commit 4c79234

Please sign in to comment.