From afc71b7df3d992ed6f3add8760fbd64b46c7cd31 Mon Sep 17 00:00:00 2001
From: Michael Rash <mbr@cipherdyne.org>
Date: Sun, 29 Jul 2012 23:31:15 -0400
Subject: [PATCH] Replay attack bug fix (encryption prefixes)

Ensure that an attacker cannot force a replay attack by intercepting an
SPA packet and the replaying it with the base64 version of "Salted__"
(for Rindael) or the "hQ" prefix (for GnuPG).  This is an important fix.
The following comment was added into the fwknopd code:

/* Ignore any SPA packets that contain the Rijndael or GnuPG prefixes
 * since an attacker might have tacked them on to a previously seen
 * SPA packet in an attempt to get past the replay check.  And, we're
 * no worse off since a legitimate SPA packet that happens to include
 * a prefix after the outer one is stripped off won't decrypt properly
 * anyway because libfko would not add a new one.
*/
---
 lib/cipher_funcs.h    |  9 ---------
 lib/fko.h             |  8 ++++++++
 server/incoming_spa.c | 14 ++++++++++++++
 test/test-fwknop.pl   | 30 ++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/lib/cipher_funcs.h b/lib/cipher_funcs.h
index 929b4a9c..06d18359 100644
--- a/lib/cipher_funcs.h
+++ b/lib/cipher_funcs.h
@@ -35,15 +35,6 @@
 #include "rijndael.h"
 #include "gpgme_funcs.h"
 
-/* Define the consistent prefixes or salt on some ecryption schemes.
- * We identify them here so we can remove and reinsert when needed.
-*/
-#define B64_RIJNDAEL_SALT "U2FsdGVkX1"
-#define B64_RIJNDAEL_SALT_STR_LEN 10
-
-#define B64_GPG_PREFIX "hQ"
-#define B64_GPG_PREFIX_STR_LEN 2
-
 /* Provide the predicted encrypted data size for given input data based
  * on a 16-byte block size (for Rijndael implementation,this also accounts
  * for the 16-byte salt as well).
diff --git a/lib/fko.h b/lib/fko.h
index 5802c5ae..56b4c3a2 100644
--- a/lib/fko.h
+++ b/lib/fko.h
@@ -194,6 +194,14 @@ typedef enum {
 #define FKO_DEFAULT_ENCRYPTION  FKO_ENCRYPTION_RIJNDAEL
 #define FKO_DEFAULT_ENC_MODE    MODE_CBC
 
+/* Define the consistent prefixes or salt on some encryption schemes.
+*/
+#define B64_RIJNDAEL_SALT "U2FsdGVkX1"
+#define B64_RIJNDAEL_SALT_STR_LEN 10
+
+#define B64_GPG_PREFIX "hQ"
+#define B64_GPG_PREFIX_STR_LEN 2
+
 /* The context holds the global state and config options, as
  * well as some intermediate results during processing. This
  * is an opaque pointer.
diff --git a/server/incoming_spa.c b/server/incoming_spa.c
index 018b914b..9bf402c0 100644
--- a/server/incoming_spa.c
+++ b/server/incoming_spa.c
@@ -62,6 +62,20 @@ preprocess_spa_data(fko_srv_options_t *opts, const char *src_ip)
     */
     spa_pkt->packet_data_len = 0;
 
+    /* Ignore any SPA packets that contain the Rijndael or GnuPG prefixes
+     * since an attacker might have tacked them on to a previously seen
+     * SPA packet in an attempt to get past the replay check.  And, we're
+     * no worse off since a legitimate SPA packet that happens to include
+     * a prefix after the outer one is stripped off won't decrypt properly
+     * anyway because libfko would not add a new one.
+    */
+    if(strncmp(ndx, B64_RIJNDAEL_SALT, B64_RIJNDAEL_SALT_STR_LEN) == 0)
+        return(SPA_MSG_NOT_SPA_DATA);
+
+    if(pkt_data_len > MIN_GNUPG_MSG_SIZE
+            && strncmp(ndx, B64_GPG_PREFIX, B64_GPG_PREFIX_STR_LEN) == 0)
+        return(SPA_MSG_NOT_SPA_DATA);
+
     /* Detect and parse out SPA data from an HTTP request. If the SPA data
      * starts with "GET /" and the user agent starts with "Fwknop", then
      * assume it is a SPA over HTTP request.
diff --git a/test/test-fwknop.pl b/test/test-fwknop.pl
index 2f5bdfdb..e645d085 100755
--- a/test/test-fwknop.pl
+++ b/test/test-fwknop.pl
@@ -1452,6 +1452,18 @@ my @tests = (
             "$fwknopdCmd $default_server_conf_args $intf_str",
         'fatal'    => $NO
     },
+    {
+        'category' => 'Rijndael SPA',
+        'subcategory' => 'client+server',
+        'detail'   => 'replay detection (Rijndael prefix)',
+        'err_msg'  => 'could not detect replay attack',
+        'function' => \&replay_detection,
+        'pkt_prefix' => 'U2FsdGVkX1'
+        'cmdline'  => $default_client_args,
+        'fwknopd_cmdline'  => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " .
+            "$fwknopdCmd $default_server_conf_args $intf_str",
+        'fatal'    => $NO
+    },
     {
         'category' => 'Rijndael SPA',
         'subcategory' => 'server',
@@ -1622,6 +1634,19 @@ my @tests = (
         'fwknopd_cmdline'  => $default_server_gpg_args,
         'fatal'    => $NO
     },
+    {
+        'category' => 'GnuPG (GPG) SPA',
+        'subcategory' => 'client+server',
+        'detail'   => 'replay detection (GnuPG prefix)',
+        'err_msg'  => 'could not detect replay attack',
+        'function' => \&replay_detection,
+        'pkt_prefix' => 'hQ'
+        'cmdline'  => $default_client_args,
+        'fwknopd_cmdline'  => "LD_LIBRARY_PATH=$lib_dir $valgrind_str " .
+            "$fwknopdCmd $default_server_conf_args $intf_str",
+        'fatal'    => $NO
+    },
+
 
     {
         'category' => 'GnuPG (GPG) SPA',
@@ -1704,6 +1729,7 @@ my %test_keys = (
     'fw_rule_created' => $OPTIONAL,
     'fw_rule_removed' => $OPTIONAL,
     'server_conf'     => $OPTIONAL,
+    'pkt_prefix'      => $OPTIONAL,
     'positive_output_matches' => $OPTIONAL,
     'negative_output_matches' => $OPTIONAL,
     'server_positive_output_matches' => $OPTIONAL,
@@ -2143,6 +2169,10 @@ sub replay_detection() {
         return 0;
     }
 
+    if ($test_hr->{'pkt_prefix'}) {
+        $spa_pkt = $test_hr->{'pkt_prefix'} . $spa_pkt;
+    }
+
     my @packets = (
         {
             'proto'  => 'udp',