On 23/04/17 10:59, Steffan Karger wrote:
> Hi,
>
> The change makes sense, so 'Feature-ACK', but one remark:
>
> On 20-04-17 16:41, Hristo Venev wrote:
>> @@ -191,16 +191,23 @@ extract_x509_field_ssl(X509_NAME *x509, const char
>> *field_name, char *out,
>> X509_NAME_ENTRY *x509ne = 0;
>> ASN1_STRING *asn1 = 0;
>> unsigned char *buf = NULL;
>> - int nid = OBJ_txt2nid(field_name);
>> + ASN1_OBJECT *obj = OBJ_txt2obj(field_name, 0);
>> +
>> + if (obj == NULL)
>> + {
>> + crypto_msg(M_FATAL, "Cannot get ASN1_OBJECT for %s", field_name);
>> + }
>
> M_FATAL is currently acceptable, because field_name always comes from
> the local config file, but I'd prefer to make this non-fatal, and return
> FAILURE. That ensures that if someone decides to use this function in a
> way where field_name *is* attacker controlled, we do not introduce a
> remote DoS.
>
> As for the log level of this message: I think D_TLS_DEBUG_LOW (--verb 3)
> or possibly D_TLS_ERRORS (--verb 1) are appropriate.Agreed! I'm leaning towards D_TLS_ERRORS over D_TLS_DEBUG_LOW. This field is normally configured via the configuration file, IIUC ( like --x509-username-field). So if this fails, it should have a very low threshold of warning the sys-admin. Normally, this shouldn't happen unless a client is truly miss-configured. -- kind regards, David Sommerseth OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
