View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0006178 | Taler | exchange | public | 2020-04-14 11:47 | 2020-06-25 21:45 |
| Reporter | fefe | Assigned To | Christian Grothoff | Resolution deadline | |
| Implementer | Reviewer | ||||
| Release signed by | Deployed by | ||||
| Verifier | Communicator | ||||
| Data protection authority notification required | No | ||||
| CSIRT notification required | No | ||||
| Estimated effort (hours) | Actual effort (hours) | 0 | |||
| Fast-track | No | ||||
| Work package | none | ||||
| Priority | normal | Severity | tweak | Reproducibility | N/A |
| Status | closed | Resolution | no change required | ||
| Product Version | 0.7.0 | ||||
| Target Version | 0.7.1 | Fixed in Version | 0.7.1 | ||
| Summary | 0006178: Why do we have length fields in fixed-length structures? | ||||
| Description | Here is a code snippet that validates if a denomination key is valid: 148 if (ntohl (dki->issue.properties.purpose.size) != 149 sizeof (struct TALER_DenominationKeyValidityPS)) 150 { 151 fprintf (stderr, 152 "Denomination key for `%s' has invalid purpose size\n", 153 alias); 154 return GNUNET_SYSERR; 155 } 156 157 if ( (0 != GNUNET_TIME_absolute_ntoh ( 158 dki->issue.properties.start).abs_value_us % 1000000) || 159 (0 != GNUNET_TIME_absolute_ntoh ( 160 dki->issue.properties.expire_withdraw).abs_value_us % 1000000) || 161 (0 != GNUNET_TIME_absolute_ntoh ( 162 dki->issue.properties.expire_legal).abs_value_us % 1000000) || 163 (0 != GNUNET_TIME_absolute_ntoh ( 164 dki->issue.properties.expire_deposit).abs_value_us % 1000000) ) 165 { 166 fprintf (stderr, 167 "Timestamps are not multiples of a round second\n"); 168 return GNUNET_SYSERR; 169 } Why do you use a time stamp format that allows representing those invalid values in the first place? Why have a length field in the first place if its value is fixed and both sender and receiver check for equality with the well-known value? This is not a security bug but it appears to be wasted effort. | ||||
| Tags | No tags attached. | ||||
| The length field is there because we insist on it being explicit whenever data is signed over, so that there is never a question of the length of the message to which a signature applies (helps against weak hash functions; we may hope our hash function is not weak, but having the length field is better). Now, _usually_ we don't send the length field (or purpose) and thus don't have to check it, but in the case of the 'issue' the serialized data structure includes the length field, and thus we need to check it when deserializing. Having a second format for the serialized version to just leave out the length field still seems worse to me. For the timestamps, well, in some contexts they must be rounded, in others not. We could introduce another data type for those that are rounded to make it more explicit if rounding is required. However, I think it would still be best to preserve the same unit instead of sometimes measuring time in seconds, sometimes milliseconds and sometimes nanoseconds. Otherwise, we would need to replicate all of the GNUNET_TIME_*-functions for the rounded time in a different unit, plus possibly functions to allow combinations of rounded and not-rounded times -- which seems like a terrible trade-off. Thus, upon serialization/deserialization we'd still need to check for rounding (well-formed), so while introducing a new type may make it a tad more explicit, it'd still not eliminate the check. | |
| Are we good on this? | |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-04-14 11:47 | fefe | New Issue | |
| 2020-04-14 11:47 | fefe | Status | new => assigned |
| 2020-04-14 11:47 | fefe | Assigned To | => Christian Grothoff |
| 2020-04-14 14:00 | Christian Grothoff | Note Added: 0015648 | |
| 2020-04-14 18:53 | Christian Grothoff | Assigned To | Christian Grothoff => |
| 2020-04-14 18:53 | Christian Grothoff | Severity | minor => tweak |
| 2020-04-14 18:53 | Christian Grothoff | Status | assigned => acknowledged |
| 2020-05-02 23:34 | Christian Grothoff | Assigned To | => Christian Grothoff |
| 2020-05-02 23:34 | Christian Grothoff | Status | acknowledged => feedback |
| 2020-05-02 23:34 | Christian Grothoff | Note Added: 0015844 | |
| 2020-06-16 17:57 | Christian Grothoff | Status | feedback => closed |
| 2020-06-16 17:57 | Christian Grothoff | Resolution | open => no change required |
| 2020-06-16 17:57 | Christian Grothoff | Target Version | => 0.8.1 |
| 2020-06-25 21:45 | Christian Grothoff | Fixed in Version | => 0.7.1 |
| 2020-06-25 21:45 | Christian Grothoff | Target Version | 0.8.1 => 0.7.1 |