-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement AES ECB, CBC and GCM modes #5
Conversation
0113306
to
cebb412
Compare
a5be7e4
to
938ad52
Compare
cebb412
to
afc8ef1
Compare
9eae00e
to
09899dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's one small fix, but otherwise LGTM.
if err != nil { | ||
panic(err) | ||
} | ||
if int(ret) != len(src) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't int(ret)
potentially truncate on 32 bit platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ret
is a uint32
, so I don't think it will overflow. What will happen if len(src) don't fit in 32 bits is that this function will panic. I guess this behavior is right, at least I won't feel comfortable batching encrypts/decrypts in 32 bit chunks unless a crypto expert says we can do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing to look at is signed values: https://go.dev/play/p/ZQXsT1GlEPQ
var v uint64 = math.MaxUint64 // 64 to match platform-sized int in playground
fmt.Printf("%x %x\n%x\n", v, int(v), math.MaxInt)
ffffffffffffffff -1
7fffffffffffffff
But I think it's ok here, because int
is 32-bit, and len
returns an int
, then the size of the slice is limited to the positive 31 bits and this will never occur. If bcrypt.Decrypt
is misbehaving and returns a large value that casts to a negative int
, that will always panic because len(src)
always returns a positivenon-negative int
. (And we want a panic in this case.)
Co-authored-by: Davis Goodin <[email protected]>
Please review #3 first.
This PR implements AES ECB, CBC and GCM modes. Each mode is implemented in its own commit.
CRT mode is not supported by CNG so it has not bee implemented, see #4.
The AES GCM TLS mode has been implemented in the same way as OpenSSL: microsoft/go-crypto-openssl#21 as CNG does not have a specific TLS mode.