Improvements in padding and unpadding:

- Fix an off-by-one error in ISO7816 unpadding.
- Unpadding errors are not assertion failures, handle the error.
- Check all the requirements of the padding scheme when unpadding.

README.rst

@@ -464,6 +464,12 @@ The failure conditions of ``.decrypt()`` are similar to those for
 * Missing ``iv`` or ``ctr`` parameters for modes of operation that
   require them.
 
+* An error in the padding bytes, if padding is specified. For example,
+  if the number of padding bytes added is longer than the cipher's
+  block length (for ``PKCS7`` and ``X923`` padding), or if a value
+  such as 0x80 or 0x00 is not found where the padding scheme requires
+  it.
+
 * Failure of the libgcrypt library.
 
 If the method fails, a message is logged using the ``VCL_Error`` tag,

src/padding.h

@@ -95,30 +95,43 @@ static pad_f * const padf[] = {
 	[X923]		= pad_x923,
 };
 
-typedef int unpadlen_f(void *plaintext, int cipherlen, size_t blocklen);
+typedef ssize_t unpadlen_f(void *plaintext, int cipherlen, size_t blocklen);
 
-static int  __match_proto__(unpadlen_f)
+static ssize_t  __match_proto__(unpadlen_f)
 unpadlen_pkcs7(void *plaintext, int cipherlen, size_t blocklen)
 {
-	assert(((unsigned char *)plaintext)[cipherlen-1] <= blocklen);
-	assert(((unsigned char *)plaintext)[cipherlen-1] <= cipherlen);
-	return cipherlen - ((unsigned char *)plaintext)[cipherlen-1];
+	uint8_t padlen = ((uint8_t *)plaintext)[cipherlen-1];
+
+	if (padlen > blocklen || padlen > cipherlen)
+		return -1;
+	for (int i = cipherlen - 2; i >= cipherlen - padlen; i--)
+		if (padlen != ((uint8_t *)plaintext)[i])
+			return -1;
+	return cipherlen - padlen;
 }
 
-static int  __match_proto__(unpadlen_f)
+static ssize_t  __match_proto__(unpadlen_f)
 unpadlen_iso7816(void *plaintext, int cipherlen, size_t blocklen)
 {
 	(void) blocklen;
 	int i = cipherlen - 1;
-	while (i > 0 && ((unsigned char *)plaintext)[i] != 0x80)
-		assert(0 == ((unsigned char *)plaintext)[i--]);
-	return i - 1;
+	while (i >= 0 && ((uint8_t *)plaintext)[i] != 0x80)
+		if (0 != ((uint8_t *)plaintext)[i--])
+			return -1;
+	return i;
 }
 
-static int  __match_proto__(unpadlen_f)
+static ssize_t  __match_proto__(unpadlen_f)
 unpadlen_x923(void *plaintext, int cipherlen, size_t blocklen)
 {
-	return unpadlen_pkcs7(plaintext, cipherlen, blocklen);
+	uint8_t padlen = ((uint8_t *)plaintext)[cipherlen-1];
+
+	if (padlen > blocklen || padlen > cipherlen)
+		return -1;
+	for (int i = cipherlen - 2; i >= cipherlen - padlen; i--)
+		if (0 != ((uint8_t *)plaintext)[i])
+			return -1;
+	return cipherlen - padlen;
 }
 
 static unpadlen_f * const unpadlenf[] = {

src/tests/padding.vtc

+# looks like -*- vcl -*-
+
+varnishtest "padding"
+
+varnish v1 -vcl {
+	import gcrypt from "${vmod_topbuild}/src/.libs/libvmod_gcrypt.so";
+	backend b { .host = "${bad_ip}"; }
+
+	sub vcl_init {
+		gcrypt.init(FINISH);
+	}
+} -start
+
+# Test padding -- encrypt with padding, decrypt with no padding, and
+# verify that the result has padding bytes as expected.
+varnish v1 -vcl {
+	import blobcode;
+	import gcrypt from "${vmod_topbuild}/src/.libs/libvmod_gcrypt.so";
+	backend b { .host = "${bad_ip}"; }
+
+	sub vcl_init {
+		new k1 = blobcode.blob(HEX, "000102030405060708090a0b0c0d0e0f");
+		new none = gcrypt.symmetric(AES, ECB, NONE, key=k1.get());
+		new pkcs7 = gcrypt.symmetric(AES, ECB, PKCS7, key=k1.get());
+		new iso7816 = gcrypt.symmetric(AES, ECB, ISO7816, key=k1.get());
+		new x923 = gcrypt.symmetric(AES, ECB, X923, key=k1.get());
+
+		new foo = blobcode.blob(encoded="foo");
+	}
+
+	sub vcl_recv {
+		return(synth(200));
+	}
+
+	sub vcl_synth {
+		set resp.http.foo-pkcs7-cipher
+		    = blobcode.encode(BASE64, pkcs7.encrypt(foo.get()));
+		set resp.http.foo-pkcs7-plain
+		    = blobcode.encode(HEXUC, none.decrypt(
+		          blobcode.decode(BASE64, resp.http.foo-pkcs7-cipher)));
+		set resp.http.foo-iso7816-cipher
+		    = blobcode.encode(BASE64, iso7816.encrypt(foo.get()));
+		set resp.http.foo-iso7816-plain
+		    = blobcode.encode(HEXUC, none.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-iso7816-cipher)));
+		set resp.http.foo-x923-cipher
+		    = blobcode.encode(BASE64, x923.encrypt(foo.get()));
+		set resp.http.foo-x923-plain
+		    = blobcode.encode(HEXUC, none.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-x923-cipher)));
+
+		set resp.http.empty-pkcs7-cipher
+		    = blobcode.encode(BASE64,
+		                    pkcs7.encrypt(blobcode.decode(encoded="")));
+		set resp.http.empty-pkcs7-plain
+		    = blobcode.encode(HEXUC, none.decrypt(
+		        blobcode.decode(BASE64, resp.http.empty-pkcs7-cipher)));
+		set resp.http.empty-iso7816-cipher
+		    = blobcode.encode(BASE64,
+		                  iso7816.encrypt(blobcode.decode(encoded="")));
+		set resp.http.empty-iso7816-plain
+		    = blobcode.encode(HEXUC, none.decrypt(
+		      blobcode.decode(BASE64, resp.http.empty-iso7816-cipher)));
+		set resp.http.empty-x923-cipher
+		    = blobcode.encode(BASE64,
+		                     x923.encrypt(blobcode.decode(encoded="")));
+		set resp.http.empty-x923-plain
+		    = blobcode.encode(HEXUC, none.decrypt(
+		        blobcode.decode(BASE64, resp.http.empty-x923-cipher)));
+		return(deliver);
+	}
+}
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.status == 200
+	expect resp.http.foo-pkcs7-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-pkcs7-plain == "666F6F0D0D0D0D0D0D0D0D0D0D0D0D0D"
+	expect resp.http.foo-iso7816-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-iso7816-plain == "666F6F80000000000000000000000000"
+	expect resp.http.foo-x923-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-x923-plain == "666F6F0000000000000000000000000D"
+	expect resp.http.empty-pkcs7-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-pkcs7-plain == "10101010101010101010101010101010"
+	expect resp.http.empty-iso7816-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-iso7816-plain == "80000000000000000000000000000000"
+	expect resp.http.empty-x923-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-x923-plain == "00000000000000000000000000000010"
+} -run
+
+# Test unpadding -- encrypt a plaintext without padding, where the
+# plaintext has the exact block length and padding bytes already
+# added. Then decrypt with padding, and verify that the result has
+# padding removed.
+varnish v1 -vcl {
+	import blobcode;
+	import gcrypt from "${vmod_topbuild}/src/.libs/libvmod_gcrypt.so";
+	backend b { .host = "${bad_ip}"; }
+
+	sub vcl_init {
+		new k1 = blobcode.blob(HEX, "000102030405060708090a0b0c0d0e0f");
+		new none = gcrypt.symmetric(AES, ECB, NONE, key=k1.get());
+		new pkcs7 = gcrypt.symmetric(AES, ECB, PKCS7, key=k1.get());
+		new iso7816 = gcrypt.symmetric(AES, ECB, ISO7816, key=k1.get());
+		new x923 = gcrypt.symmetric(AES, ECB, X923, key=k1.get());
+
+		new foo_pkcs7
+		    = blobcode.blob(HEX, "666F6F0D0D0D0D0D0D0D0D0D0D0D0D0D");
+		new foo_iso7816
+		    = blobcode.blob(HEX, "666F6F80000000000000000000000000");
+		new foo_x923
+		    = blobcode.blob(HEX, "666F6F0000000000000000000000000D");
+
+		new empty_pkcs7
+		    = blobcode.blob(HEX, "10101010101010101010101010101010");
+		new empty_iso7816
+		    = blobcode.blob(HEX, "80000000000000000000000000000000");
+		new empty_x923
+		    = blobcode.blob(HEX, "00000000000000000000000000000010");
+	}
+
+	sub vcl_recv {
+		return(synth(200));
+	}
+
+	sub vcl_synth {
+		set resp.http.foo-pkcs7-cipher
+		    = blobcode.encode(BASE64, none.encrypt(foo_pkcs7.get()));
+		set resp.http.foo-pkcs7-plain
+		    = blobcode.encode(blob=pkcs7.decrypt(
+		          blobcode.decode(BASE64, resp.http.foo-pkcs7-cipher)));
+		set resp.http.foo-iso7816-cipher
+		    = blobcode.encode(BASE64, none.encrypt(foo_iso7816.get()));
+		set resp.http.foo-iso7816-plain
+		    = blobcode.encode(blob=iso7816.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-iso7816-cipher)));
+		set resp.http.foo-x923-cipher
+		    = blobcode.encode(BASE64, none.encrypt(foo_x923.get()));
+		set resp.http.foo-x923-plain
+		    = blobcode.encode(blob=x923.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-x923-cipher)));
+
+		set resp.http.empty-pkcs7-cipher
+		    = blobcode.encode(BASE64, none.encrypt(empty_pkcs7.get()));
+		set resp.http.empty-pkcs7-plain
+		    = blobcode.encode(blob=pkcs7.decrypt(
+		        blobcode.decode(BASE64, resp.http.empty-pkcs7-cipher)));
+		set resp.http.empty-iso7816-cipher
+		   = blobcode.encode(BASE64, none.encrypt(empty_iso7816.get()));
+		set resp.http.empty-iso7816-plain
+		    = blobcode.encode(blob=iso7816.decrypt(
+		      blobcode.decode(BASE64, resp.http.empty-iso7816-cipher)));
+		set resp.http.empty-x923-cipher
+		    = blobcode.encode(BASE64, none.encrypt(empty_x923.get()));
+		set resp.http.empty-x923-plain
+		    = blobcode.encode(blob=x923.decrypt(
+		        blobcode.decode(BASE64, resp.http.empty-x923-cipher)));
+		return(deliver);
+	}
+}
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.status == 200
+	expect resp.http.foo-pkcs7-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-pkcs7-plain == "foo"
+	expect resp.http.foo-iso7816-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-iso7816-plain == "foo"
+	expect resp.http.foo-x923-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-x923-plain == "foo"
+	expect resp.http.empty-pkcs7-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-pkcs7-plain == ""
+	expect resp.http.empty-iso7816-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-iso7816-plain == ""
+	expect resp.http.empty-x923-cipher ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-x923-plain == ""
+} -run
+
+# Padding errors
+varnish v1 -vcl {
+	import blobcode;
+	import gcrypt from "${vmod_topbuild}/src/.libs/libvmod_gcrypt.so";
+	backend b { .host = "${bad_ip}"; }
+
+	sub vcl_init {
+		new k1 = blobcode.blob(HEX, "000102030405060708090a0b0c0d0e0f");
+		new none = gcrypt.symmetric(AES, ECB, NONE, key=k1.get());
+		new pkcs7 = gcrypt.symmetric(AES, ECB, PKCS7, key=k1.get());
+		new iso7816 = gcrypt.symmetric(AES, ECB, ISO7816, key=k1.get());
+		new x923 = gcrypt.symmetric(AES, ECB, X923, key=k1.get());
+
+		new foo_pkcs7_1
+		    = blobcode.blob(HEX, "666F6F0E0D0D0D0D0D0D0D0D0D0D0D0D");
+		new foo_pkcs7_2
+		    = blobcode.blob(HEX, "666F6F0D0D0D0D0D0D0D0D0D0D0D0D11");
+		new foo_iso7816_1
+		    = blobcode.blob(HEX, "666F6F80000000000001000000000000");
+		new foo_iso7816_2
+		    = blobcode.blob(HEX, "666F6F00000000000000000000000000");
+		new foo_x923_1
+		    = blobcode.blob(HEX, "666F6F0000000001000000000000000D");
+		new foo_x923_2
+		    = blobcode.blob(HEX, "666F6F00000000000000000000000011");
+
+		new empty_pkcs7_1
+		    = blobcode.blob(HEX, "0F101010101010101010101010101010");
+		new empty_pkcs7_2
+		    = blobcode.blob(HEX, "10101010101010101010101010101011");
+		new empty_iso7816_1
+		    = blobcode.blob(HEX, "00000000000000000000000000000000");
+		new empty_iso7816_2
+		    = blobcode.blob(HEX, "80010000000000000000000000000000");
+		new empty_x923_1
+		    = blobcode.blob(HEX, "01000000000000000000000000000010");
+		new empty_x923_2
+		    = blobcode.blob(HEX, "00000000000000000000000000000011");
+	}
+
+	sub vcl_recv {
+		return(synth(200));
+	}
+
+	sub vcl_synth {
+		set resp.http.foo-pkcs7-cipher-1
+		    = blobcode.encode(BASE64, none.encrypt(foo_pkcs7_1.get()));
+		set resp.http.foo-pkcs7-plain-1
+		    = blobcode.encode(blob=pkcs7.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-pkcs7-cipher-1)));
+		set resp.http.foo-pkcs7-cipher-2
+		    = blobcode.encode(BASE64, none.encrypt(foo_pkcs7_2.get()));
+		set resp.http.foo-pkcs7-plain-2
+		    = blobcode.encode(blob=pkcs7.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-pkcs7-cipher-2)));
+		set resp.http.foo-iso7816-cipher-1
+		   = blobcode.encode(BASE64, none.encrypt(foo_iso7816_1.get()));
+		set resp.http.foo-iso7816-plain-1
+		    = blobcode.encode(blob=iso7816.decrypt(
+		      blobcode.decode(BASE64, resp.http.foo-iso7816-cipher-1)));
+		set resp.http.foo-iso7816-cipher-2
+		   = blobcode.encode(BASE64, none.encrypt(foo_iso7816_2.get()));
+		set resp.http.foo-iso7816-plain-2
+		    = blobcode.encode(blob=iso7816.decrypt(
+		      blobcode.decode(BASE64, resp.http.foo-iso7816-cipher-2)));
+		set resp.http.foo-x923-cipher-1
+		    = blobcode.encode(BASE64, none.encrypt(foo_x923_1.get()));
+		set resp.http.foo-x923-plain-1
+		    = blobcode.encode(blob=x923.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-x923-cipher-1)));
+		set resp.http.foo-x923-cipher-2
+		    = blobcode.encode(BASE64, none.encrypt(foo_x923_1.get()));
+		set resp.http.foo-x923-plain-2
+		    = blobcode.encode(blob=x923.decrypt(
+		        blobcode.decode(BASE64, resp.http.foo-x923-cipher-2)));
+
+		set resp.http.empty-pkcs7-cipher-1
+		   = blobcode.encode(BASE64, none.encrypt(empty_pkcs7_1.get()));
+		set resp.http.empty-pkcs7-plain-1
+		    = blobcode.encode(blob=pkcs7.decrypt(
+		      blobcode.decode(BASE64, resp.http.empty-pkcs7-cipher-1)));
+		set resp.http.empty-pkcs7-cipher-2
+		   = blobcode.encode(BASE64, none.encrypt(empty_pkcs7_2.get()));
+		set resp.http.empty-pkcs7-plain-2
+		    = blobcode.encode(blob=pkcs7.decrypt(
+		      blobcode.decode(BASE64, resp.http.empty-pkcs7-cipher-2)));
+		set resp.http.empty-iso7816-cipher-1
+		 = blobcode.encode(BASE64, none.encrypt(empty_iso7816_1.get()));
+		set resp.http.empty-iso7816-plain-1
+		    = blobcode.encode(blob=iso7816.decrypt(
+		    blobcode.decode(BASE64, resp.http.empty-iso7816-cipher-1)));
+		set resp.http.empty-iso7816-cipher-2
+		 = blobcode.encode(BASE64, none.encrypt(empty_iso7816_2.get()));
+		set resp.http.empty-iso7816-plain-2
+		    = blobcode.encode(blob=iso7816.decrypt(
+		    blobcode.decode(BASE64, resp.http.empty-iso7816-cipher-2)));
+		set resp.http.empty-x923-cipher-1
+		    = blobcode.encode(BASE64, none.encrypt(empty_x923_1.get()));
+		set resp.http.empty-x923-plain-1
+		    = blobcode.encode(blob=x923.decrypt(
+		       blobcode.decode(BASE64, resp.http.empty-x923-cipher-1)));
+		set resp.http.empty-x923-cipher-2
+		    = blobcode.encode(BASE64, none.encrypt(empty_x923_1.get()));
+		set resp.http.empty-x923-plain-2
+		    = blobcode.encode(blob=x923.decrypt(
+		       blobcode.decode(BASE64, resp.http.empty-x923-cipher-2)));
+		return(deliver);
+	}
+}
+
+client c1 {
+	txreq
+	rxresp
+	expect resp.status == 200
+	expect resp.http.foo-pkcs7-cipher-1 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-pkcs7-plain-1 == ""
+	expect resp.http.foo-pkcs7-cipher-2 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-pkcs7-plain-2 == ""
+	expect resp.http.foo-iso7816-cipher-1 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-iso7816-plain-1 == ""
+	expect resp.http.foo-iso7816-cipher-2 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-iso7816-plain-2 == ""
+	expect resp.http.foo-x923-cipher-1 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-x923-plain-1 == ""
+	expect resp.http.foo-x923-cipher-2 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.foo-x923-plain-2 == ""
+	expect resp.http.empty-pkcs7-cipher-1 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-pkcs7-plain-1 == ""
+	expect resp.http.empty-pkcs7-cipher-2 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-pkcs7-plain-2 == ""
+	expect resp.http.empty-iso7816-cipher-1 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-iso7816-plain-1 == ""
+	expect resp.http.empty-iso7816-cipher-2 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-iso7816-plain-2 == ""
+	expect resp.http.empty-x923-cipher-1 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-x923-plain-1 == ""
+	expect resp.http.empty-x923-cipher-2 ~ "^[A-Za-z0-9+/]+=*$"
+	expect resp.http.empty-x923-plain-2 == ""
+} -run
+
+logexpect l1 -v v1 -d 1 -q "VCL_Error" {
+	expect 0 * Begin req
+	expect * = VCL_Error "^vmod gcrypt error: in pkcs7.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in pkcs7.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in iso7816.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in iso7816.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in x923.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in x923.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in pkcs7.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in pkcs7.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in iso7816.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in iso7816.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in x923.decrypt..: incorrect padding$"
+	expect * = VCL_Error "^vmod gcrypt error: in x923.decrypt..: incorrect padding$"
+	expect * = End
+} -run
+

src/vmod_gcrypt.c

@@ -526,6 +526,13 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 			(unpadlenf[symmetric->padding])(plaintext->priv,
 							ciphertext->len,
 							symmetric->blocklen);
+		if (plaintext->len < 0) {
+			VERR(ctx, "in %s.decrypt(): incorrect padding",
+			     symmetric->vcl_name);
+			WS_Release(ctx->ws, 0);
+			WS_Reset(ctx->ws, snap);
+			return NULL;
+		}
 	}
 	WS_Release(ctx->ws, plaintext->len);
 	plaintext->free = NULL;

src/vmod_gcrypt.vcc

@@ -413,6 +413,12 @@ The failure conditions of ``.decrypt()`` are similar to those for
 * Missing ``iv`` or ``ctr`` parameters for modes of operation that
   require them.
 
+* An error in the padding bytes, if padding is specified. For example,
+  if the number of padding bytes added is longer than the cipher's
+  block length (for ``PKCS7`` and ``X923`` padding), or if a value
+  such as 0x80 or 0x00 is not found where the padding scheme requires
+  it.
+
 * Failure of the libgcrypt library.
 
 If the method fails, a message is logged using the ``VCL_Error`` tag,