aescbc unpad function removes too much padding

[tomhoward87]

I think there may be a bug in the following function in the aescbc package - I'm not an expert so this may be an issue on the provider end (unfortunately I am unable to share actual examples or more details due to confidentiality) but I managed to pin it down to the following function:

func unpad(buf []byte, n int) ([]byte, error) {
    [...]
	count := 0
	last := buf[lbuf-1]
	for i := lbuf - 1; i >= 0; i-- {
		if buf[i] != last {
			break
		}
		count++
	}
	if count != int(last) {
		return nil, fmt.Errorf(`invalid padding`)
	}
    [...]
}

The padding is made up of the number of bytes e.g. the last 9 bytes are all 9, or the last 11 bytes are all 11. However if the last non-padding byte also happens to match the byte in question it is also removed and an error is returned e.g.

data [.... 256 253 5 ] padding [5 5 5 5 5]

the last 5 in the data is treated as padding when it shouldn't be.

Apologies if this is expected behaviour and the fault lies with the provider for using an incorrect standard but I thought I'd check with you first.

$ go version
go version go1.17.1 linux/amd64

[lestrrat]

oooooh... hmm, that piece of code looks like something that came out of the very very very first generation of my code (in fact it seems to exist in code from at least 5 years ago, probably a bit older). And I really don't remember if I just copied from elsewhere or I came up with it, but reading the PKCS#7 padding behavior once again, I think you might be right

Are you using v2? If so does this fix your problem?

diff --git a/jwe/internal/aescbc/aescbc.go b/jwe/internal/aescbc/aescbc.go
index 500325f..457abb8 100644
--- a/jwe/internal/aescbc/aescbc.go
+++ b/jwe/internal/aescbc/aescbc.go
@@ -44,6 +44,9 @@ func unpad(buf []byte, n int) ([]byte, error) {
                        break
                }
                count++
+               if count >= int(last) {
+                       break
+               }
        }
        if count != int(last) {
                return nil, fmt.Errorf(`invalid padding`)

[tomhoward87]

@lestrrat Yes, that fixes it, thanks for getting back to me so quickly!

[lestrrat]

Okay, well I'm glad it resolved your issue, but let me think about how to release fixed versions, as I haven't checked the status of v1 yet.

[lestrrat]

Releases made. Thank you!