[mod_gnutls-devel] [PATCH 2/2] implement GnuTLSExportCertificates control over max exported cert size

[Benny Baumann]

Hi Daniel,

Am 24.02.2014 15:37, schrieb Daniel Kahn Gillmor:
> Hi Benny--
>
> On 02/24/2014 03:45 AM, Benny Baumann wrote:
>
>> Some small notes I'll interleave with the shortened diff for simplicity.
> thanks for the review!  is there a reason that it's off-list?  I'd love
> to have this sort of thing on-list.
was accidentially off-list. I'd have marked it in the subject or
otherwise noted to be off-list if intended. So no problem with
republishing it.
> For now, my reply is private because i generally don't like forwarding
> people's private comments to public lists without permission.
>
> but if you're OK with this exchange being public, i'm happy to republish
> it.  let me know!
>
>>> +        while (apr_isspace(*endptr)) endptr++;
>>> +        if (*endptr == '\0' || *endptr == 'b' || *endptr == 'B') {
>>> +            ;
>>> +        } else if (*endptr == 'k' || *endptr == 'K') {
>>> +            sc->export_certificates_size *= 1024;
>>> +        } else {
>>> +            return "GnuTLSExportCertificates must be set to a size (in bytes) or 'On' or 'Off'";
>>> +        }
>> Works for the common case but allows 64KBoom and other values which
>> should throw a warning.
> I think you're saying that it doesn't check that byte *after* the B is a
> null byte. (do we also want to allow a series of whitespace followed by
> a null byte?).  That's a good catch.  Looking at upstream apache code, i
> think there are probably other directives that can be tricked like this.
At least we should define it properly. I thing the following should be
acceptable:
16384 16K 16KB 16KiB 16kB 16M (and variants).
Trailing whitespace might be ignored, non-whitespace should trigger a
warning.
As should any other invalid value causing the default to be used.
>>> +        } else {
>>> +            ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
>>> +                          "GnuTLS: dazed and confused about X.509 certificate size");
>> When can this warning happen?
> That's a great question!  The answer is "i don't know" -- i expect it to
> never trigger, since i don't see how gnutls_x509_crt_export() can return
> anything but GNUTLS_E_SHORT_MEMORY_BUFFER when passed an output buffer
> of size 0.
>
> That said, the code needs to verify that its assumption is correct, and
> so it will log this failed assumption.
>
> Do you have any suggestions for other ways we should handle this?
No. Just be a bit more clear in the message so people who didn't read
the patch know what's going on.
>>> +        } else {
>>> +            ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
>>> +                          "GnuTLS: dazed and confused about OpenPGP certificate size");
>> Same question here: How to trigger this one?
> Same answer as above, but for gnutls_openpgp_crt_export() instead.
Guessed so ;-)
>>> -    "Whether to export PEM encoded certificates to CGIs. Default: Off"),
>>> +    "Max size to export PEM encoded certificates to CGIs (or off to disable). Default: off"),
>> Inconsistency with other descriptions. Off with capital O for all the
>> other texts.
> Ah, so it is.  but doc/mod_gnutls_manual.mdwn uses all lowercase.  We
> should probably standardize across them both.  Thanks for catching this
> inconsistency.
AFAIR Apache uses first letter uppercase in its documentation and I
think we should in the module do the same. But I'm indifferent about
either way ;-) Yet a slight tendency to keep to the Apache way exists ...
>
> 	--dkg
Regards,
BenBE.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: </pipermail/attachments/20140225/eaff1d01/attachment.sig>