DNSSEC integer encoding
Proper integer encoding in signature and public key conversion operations.
Closes #357 (closed) #358 (closed)
Merge request reports
Activity
@dsalzman feel free to co-review
56 64 assert(rdata); 57 65 58 66 _cleanup_datum_ gnutls_datum_t modulus = { 0 }; 59 _cleanup_datum_ gnutls_datum_t pub_exponent = { 0 }; 67 _cleanup_datum_ gnutls_datum_t exponent = { 0 }; 60 68 61 int result = gnutls_pubkey_get_pk_rsa_raw(key, &modulus, &pub_exponent); 69 int result = gnutls_pubkey_get_pk_rsa_raw(key, &modulus, &exponent); 115 size_t g_size = bignum_size_u_datum(&g); 116 size_t y_size = bignum_size_u_datum(&y); 111 117 112 if (q.size != 20) { 118 if (q_size != 20) { 113 119 // only certain key size range can be exported in DNSKEY 114 120 return DNSSEC_INVALID_KEY_SIZE; 115 121 } 116 122 117 assert(p.size == g.size && g.size == y.size); 118 assert(p.size >= 64 && (p.size - 64) % 8 == 0); 123 if (p_size != g_size || g_size != y_size) { 124 return DNSSEC_INVALID_KEY_SIZE; 125 } 126 127 if (p_size < 64 || (p_size - 64) % 8 != 0) { 287 295 288 296 // parse q 289 297 290 _cleanup_datum_ gnutls_datum_t q = { 0 }; 291 q.size = 20; 292 q.data = gnutls_malloc(q.size); 293 if (!q.data) { 294 return DNSSEC_ENOMEM; 295 } 296 wire_read_datum(&ctx, &q); 298 gnutls_datum_t q = wire_take_datum(&ctx, 20); I quite disagree. All the DSA math in this module is written in-place without constants. The size of Q determines the sizes of other parameters, which are fixed by the RFC. The value really cannot change. It would break everything else. I think this would be like writing
#define TWENTY 20.I don't know - it's quite easy to lookup implementation spec for DNSSEC algorithms: https://www.iana.org/assignments/dns-sec-alg-numbers/dns-sec-alg-numbers.xhtml#dns-sec-alg-numbers-1
But OK, I can add the list of RFCs into the header.
This is https://tools.ietf.org/html/rfc2536#section-2, btw.
137 dnssec_binary_ltrim(&value_s); 127 size_t r_size = bignum_size_u(&value_r); 128 size_t s_size = bignum_size_u(&value_s); 138 129 139 if (value_r.size > 20 || value_s.size > 20) { 130 if (r_size > 20 || s_size > 20) { 140 131 return DNSSEC_MALFORMED_DATA; 141 132 } 142 133 143 134 uint8_t value_t = dsa_dnskey_get_t_value(ctx->key); 144 135 145 size_t size = 41; 146 uint8_t *data = malloc(size); 147 if (!data) { 148 return DNSSEC_ENOMEM; 136 result = dnssec_binary_alloc(dnssec, 41); -
OK, I think this is stupid. I don't want to bash you over silly things, but your over-usage of hardcoded constants makes my head spin. Couple of reasons:
-
Using macros for more-than-once-used constants makes it easier to grep for them.
-
Using macros for more-than-once-used constants makes it easier to understand what the code does. What if two completely different parts of the code use 'by chance' the same constant? If it is a macro, we don't care, we can read what the macro stands for. If it is just a number, it may be a typo, it may be the same constant, it may be random...
-
41 is premature micro-optimization for 1 + 2 * 20. If you used a macro, it wouldn't be a puzzle.
And I bet better and more experienced programmers than myself could bash you even more for this.
-
-
Or not. While I can find a general consensus[1] about magic numbers being mostly bad and should be avoided, can you show me the opposite? And it's not bikeshedding either, since it affects several places in the source and I'm not even done with the review. It is about as important as coding style and I get nitpicked for that.
[1] http://stackoverflow.com/questions/47882/what-is-a-magic-number-and-why-is-it-bad
