Closed sippejw closed 2 years ago
Overall I'm supportive of this change, but I think that we need to verify that callers in ZCrypto and ZGrab don't check that the result is
nil
to determine whether there is an error.
I believe all of the callers are in ZCrypto and can be found here: https://github.com/zmap/zcrypto/blob/4dfcec6e9a8c2014f73dd584e64dc797129d77b8/tls/key_agreement.go#L155 https://github.com/zmap/zcrypto/blob/4dfcec6e9a8c2014f73dd584e64dc797129d77b8/tls/key_agreement.go#L609 https://github.com/zmap/zcrypto/blob/4dfcec6e9a8c2014f73dd584e64dc797129d77b8/tls/key_agreement.go#L754
When config.InsecureSkipVerify
is false
the error from verifyParameters
is returned. The digest is stored in the serverKeyExchangeMsg
struct and it does not look like it is referenced again (until it is logged). Additionally, I have provided two ZGrab outputs, one where the digest is returned and one where it is not. Below are snippets, attached are the full outputs.
Current output (return nil, ...
):
...
"server_key_exchange": {
"ecdh_params": {
"curve_id": {
"name": "secp256r1",
"id": 23
},
"server_public": {
"x": {
"value": "oqTcCPUxdLhAxi5w8Xf0MfxzGlKHOcZwyxAkWZ2ZXSM=",
"length": 256
},
"y": {
"value": "6liIQR3iomft8TKydchJ8UUdKE/j02xAblJEhjiF+sA=",
"length": 256
}
}
},
"signature": {
"raw": "hFIkr0GRj8ZQkJurnRutyl//Rl2KpFWsa+RqjHU7cQPtMBWIKtlIdvvB7I2zcZgUsYpmd9BjzwxNThBWODpEVUuSBiQG08461DiWj10Aur+Fa6Bm+OAY/GyqxW2siwfy0/wvNoC/l2PUdrTPOJG//0pThHfp4jtxQqgmUqSO0vB0I5o83NwmVSJmTbMqRUhan/OZ7lQv4/X7N1JR3eW4Ijm3u50if02WLxO68U9qkK9vNzcb0JOljh2Y1otNidmFZX0aknaasJAN9mi5jgq1oeyfKCwzkpc7nxjx7STVotLX/b07/Smw05L9htOGVpBO/EMAvBgHIExCQmW+GBEZ9A==",
"type": "rsa",
"valid": false,
"signature_and_hash_type": {
"signature_algorithm": "rsa",
"hash_algorithm": "sha256"
},
"tls_version": {
"name": "TLSv1.2",
"value": 771
}
},
"signature_error": "crypto/rsa: verification error"
},
...
New output (return digest, ...
):
...
"server_key_exchange": {
"ecdh_params": {
"curve_id": {
"name": "secp256r1",
"id": 23
},
"server_public": {
"x": {
"value": "v447+PqGUuMEWd1Q3JLnQJI5ehHBOmqTNis8vPMCBZQ=",
"length": 256
},
"y": {
"value": "8nU+cBVl7JX9GPsgb8VNJMfvwAlowKaqlkjk0wMN9+c=",
"length": 256
}
}
},
"digest": "1KB+yDNaRWcrnMja2Q+TiPXeCsyS9plqBn33bQnY/5I=",
"signature": {
"raw": "UP2+K6E1DpHVhRKCHD9Z0cjU0leq5SxL/6GJCdZYrpK0DVXx3INVnrCBbqTaZugBRN6BVrId7COQeTVxCM3FJ/QIjuYF6SehV5XXXNmeeJGlVrKRhU+gsqx8A8JZbDroIkRdsq+RQ7L6Y4w5zN+QoXTTAe9r0Nk0pcrc8ffVopxoj37WvvbGlz5xJTtZ6rr+6G2fpYryrYjCvemTdhQK+a0RVX5TZ7imTb+QGg3MnMwY/B+c3QOVW6SA6u/hvv9yveEPU3rhLBLCNf3T+kmwyugdjM6QqiiXRM/AF2pDlZNEY4cyWKr+jHQ4teobyFtxhMam6z/HUKadGhz986VrEQ==",
"type": "rsa",
"valid": false,
"signature_and_hash_type": {
"signature_algorithm": "rsa",
"hash_algorithm": "sha256"
},
"tls_version": {
"name": "TLSv1.2",
"value": 771
}
},
"signature_error": "crypto/rsa: verification error"
},
...
Overall I'm supportive of this change, but I think that we need to verify that callers in ZCrypto and ZGrab don't check that the result is
nil
to determine whether there is an error.