Download raw body.
isakmpd: don't reach into ASN1_STRING [step 2/2]
On Fri, Dec 05, 2025 at 07:54:20AM +0100, Theo Buehler wrote:
> On Tue, Nov 25, 2025 at 07:07:31AM +0100, Theo Buehler wrote:
> > On Mon, Nov 24, 2025 at 03:31:58PM +0100, Theo Buehler wrote:
> > > beck and davidben have plans to make ASN1_STRING opaque in OpenSSL 4 [1].
> > > This breaks somewhere in the vicinity of 200 ports, so I don't think we'll
> > > flip the switch anytime soon in libressl, but I want base to be ready for
> > > that since that makes my life easier.
> >
> > Here's the second step. This allows things that aren't really kosher in
> > modern X.509 like omitting seconds or allowing local times.
> >
> > Since this transforms notBefore and notAfter into a string to be used in
> > a keynote assertion, existing libcrypto interfaces aren't suitable, so I
> > kept this mechanical by assigning the data and length fields to local
> > variables accessors and replacing tm->data by data and tm->length by
> > len, only occasionally fixing up whitespace and omitting parentheses.
> >
> > The whole dance should be deduplicated, but I'll leave that for someone
> > else.
>
> Anyone? It's a very simple diff... It's not my fault that it is so big :)
Looks good to me. I agree with your comment above about deduplication.
This code is spaghetti of the bad kind.
OK claudio@ but I did not test this. I no longer use isakmpd.
> Index: x509.c
> ===================================================================
> RCS file: /cvs/src/sbin/isakmpd/x509.c,v
> diff -u -p -r1.127 x509.c
> --- x509.c 25 Nov 2025 05:12:44 -0000 1.127
> +++ x509.c 25 Nov 2025 05:43:37 -0000
> @@ -116,7 +116,8 @@ x509_generate_kn(int id, X509 *cert)
> time_t tt;
> char before[15], after[15], *timecomp, *timecomp2;
> ASN1_TIME *tm;
> - int i;
> + const unsigned char *data;
> + int i, len;
>
> LOG_DBG((LOG_POLICY, 90,
> "x509_generate_kn: generating KeyNote policy for certificate %p",
> @@ -220,8 +221,8 @@ x509_generate_kn(int id, X509 *cert)
> key = NULL;
>
> if (((tm = X509_get_notBefore(cert)) == NULL) ||
> - (tm->type != V_ASN1_UTCTIME &&
> - tm->type != V_ASN1_GENERALIZEDTIME)) {
> + (ASN1_STRING_type(tm) != V_ASN1_UTCTIME &&
> + ASN1_STRING_type(tm) != V_ASN1_GENERALIZEDTIME)) {
> struct tm *ltm;
>
> tt = time(NULL);
> @@ -233,16 +234,18 @@ x509_generate_kn(int id, X509 *cert)
> strftime(before, 14, "%Y%m%d%H%M%S", ltm);
> timecomp = "LocalTimeOfDay";
> } else {
> - if (tm->data[tm->length - 1] == 'Z') {
> + data = ASN1_STRING_get0_data(tm);
> + len = ASN1_STRING_length(tm);
> + if (data[len - 1] == 'Z') {
> timecomp = "GMTTimeOfDay";
> - i = tm->length - 2;
> + i = len - 2;
> } else {
> timecomp = "LocalTimeOfDay";
> - i = tm->length - 1;
> + i = len - 1;
> }
>
> for (; i >= 0; i--) {
> - if (tm->data[i] < '0' || tm->data[i] > '9') {
> + if (data[i] < '0' || data[i] > '9') {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid data in "
> "NotValidBefore time field"));
> @@ -250,64 +253,64 @@ x509_generate_kn(int id, X509 *cert)
> }
> }
>
> - if (tm->type == V_ASN1_UTCTIME) {
> - if ((tm->length < 10) || (tm->length > 13)) {
> + if (ASN1_STRING_type(tm) == V_ASN1_UTCTIME) {
> + if (len < 10 || len > 13) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid length "
> "of NotValidBefore time field (%d)",
> - tm->length));
> + len));
> goto fail;
> }
> /* Validity checks. */
> - if ((tm->data[2] != '0' && tm->data[2] != '1') ||
> - (tm->data[2] == '0' && tm->data[3] == '0') ||
> - (tm->data[2] == '1' && tm->data[3] > '2') ||
> - (tm->data[4] > '3') ||
> - (tm->data[4] == '0' && tm->data[5] == '0') ||
> - (tm->data[4] == '3' && tm->data[5] > '1') ||
> - (tm->data[6] > '2') ||
> - (tm->data[6] == '2' && tm->data[7] > '3') ||
> - (tm->data[8] > '5')) {
> + if ((data[2] != '0' && data[2] != '1') ||
> + (data[2] == '0' && data[3] == '0') ||
> + (data[2] == '1' && data[3] > '2') ||
> + (data[4] > '3') ||
> + (data[4] == '0' && data[5] == '0') ||
> + (data[4] == '3' && data[5] > '1') ||
> + (data[6] > '2') ||
> + (data[6] == '2' && data[7] > '3') ||
> + (data[8] > '5')) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid value in "
> "NotValidBefore time field"));
> goto fail;
> }
> /* Stupid UTC tricks. */
> - if (tm->data[0] < '5')
> + if (data[0] < '5')
> snprintf(before, sizeof before, "20%s",
> - tm->data);
> + data);
> else
> snprintf(before, sizeof before, "19%s",
> - tm->data);
> - } else { /* V_ASN1_GENERICTIME */
> - if ((tm->length < 12) || (tm->length > 15)) {
> + data);
> + } else { /* V_ASN1_GENERALIZEDTIME */
> + if (len < 12 || len > 15) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid length of "
> "NotValidBefore time field (%d)",
> - tm->length));
> + len));
> goto fail;
> }
> /* Validity checks. */
> - if ((tm->data[4] != '0' && tm->data[4] != '1') ||
> - (tm->data[4] == '0' && tm->data[5] == '0') ||
> - (tm->data[4] == '1' && tm->data[5] > '2') ||
> - (tm->data[6] > '3') ||
> - (tm->data[6] == '0' && tm->data[7] == '0') ||
> - (tm->data[6] == '3' && tm->data[7] > '1') ||
> - (tm->data[8] > '2') ||
> - (tm->data[8] == '2' && tm->data[9] > '3') ||
> - (tm->data[10] > '5')) {
> + if ((data[4] != '0' && data[4] != '1') ||
> + (data[4] == '0' && data[5] == '0') ||
> + (data[4] == '1' && data[5] > '2') ||
> + (data[6] > '3') ||
> + (data[6] == '0' && data[7] == '0') ||
> + (data[6] == '3' && data[7] > '1') ||
> + (data[8] > '2') ||
> + (data[8] == '2' && data[9] > '3') ||
> + (data[10] > '5')) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid value in "
> "NotValidBefore time field"));
> goto fail;
> }
> - snprintf(before, sizeof before, "%s", tm->data);
> + snprintf(before, sizeof before, "%s", data);
> }
>
> /* Fix missing seconds. */
> - if (tm->length < 12) {
> + if (len < 12) {
> before[12] = '0';
> before[13] = '0';
> }
> @@ -317,8 +320,8 @@ x509_generate_kn(int id, X509 *cert)
>
> tm = X509_get_notAfter(cert);
> if (tm == NULL ||
> - (tm->type != V_ASN1_UTCTIME &&
> - tm->type != V_ASN1_GENERALIZEDTIME)) {
> + (ASN1_STRING_type(tm) != V_ASN1_UTCTIME &&
> + ASN1_STRING_type(tm) != V_ASN1_GENERALIZEDTIME)) {
> struct tm *ltm;
>
> tt = time(0);
> @@ -330,16 +333,18 @@ x509_generate_kn(int id, X509 *cert)
> strftime(after, 14, "%Y%m%d%H%M%S", ltm);
> timecomp2 = "LocalTimeOfDay";
> } else {
> - if (tm->data[tm->length - 1] == 'Z') {
> + data = ASN1_STRING_get0_data(tm);
> + len = ASN1_STRING_length(tm);
> + if (data[len - 1] == 'Z') {
> timecomp2 = "GMTTimeOfDay";
> - i = tm->length - 2;
> + i = len - 2;
> } else {
> timecomp2 = "LocalTimeOfDay";
> - i = tm->length - 1;
> + i = len - 1;
> }
>
> for (; i >= 0; i--) {
> - if (tm->data[i] < '0' || tm->data[i] > '9') {
> + if (data[i] < '0' || data[i] > '9') {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid data in "
> "NotValidAfter time field"));
> @@ -347,64 +352,64 @@ x509_generate_kn(int id, X509 *cert)
> }
> }
>
> - if (tm->type == V_ASN1_UTCTIME) {
> - if ((tm->length < 10) || (tm->length > 13)) {
> + if (ASN1_STRING_type(tm) == V_ASN1_UTCTIME) {
> + if (len < 10 || len > 13) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid length of "
> "NotValidAfter time field (%d)",
> - tm->length));
> + len));
> goto fail;
> }
> /* Validity checks. */
> - if ((tm->data[2] != '0' && tm->data[2] != '1') ||
> - (tm->data[2] == '0' && tm->data[3] == '0') ||
> - (tm->data[2] == '1' && tm->data[3] > '2') ||
> - (tm->data[4] > '3') ||
> - (tm->data[4] == '0' && tm->data[5] == '0') ||
> - (tm->data[4] == '3' && tm->data[5] > '1') ||
> - (tm->data[6] > '2') ||
> - (tm->data[6] == '2' && tm->data[7] > '3') ||
> - (tm->data[8] > '5')) {
> + if ((data[2] != '0' && data[2] != '1') ||
> + (data[2] == '0' && data[3] == '0') ||
> + (data[2] == '1' && data[3] > '2') ||
> + (data[4] > '3') ||
> + (data[4] == '0' && data[5] == '0') ||
> + (data[4] == '3' && data[5] > '1') ||
> + (data[6] > '2') ||
> + (data[6] == '2' && data[7] > '3') ||
> + (data[8] > '5')) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid value in "
> "NotValidAfter time field"));
> goto fail;
> }
> /* Stupid UTC tricks. */
> - if (tm->data[0] < '5')
> + if (data[0] < '5')
> snprintf(after, sizeof after, "20%s",
> - tm->data);
> + data);
> else
> snprintf(after, sizeof after, "19%s",
> - tm->data);
> - } else { /* V_ASN1_GENERICTIME */
> - if ((tm->length < 12) || (tm->length > 15)) {
> + data);
> + } else { /* V_ASN1_GENERALIZEDTIME */
> + if (len < 12 || len > 15) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid length of "
> "NotValidAfter time field (%d)",
> - tm->length));
> + len));
> goto fail;
> }
> /* Validity checks. */
> - if ((tm->data[4] != '0' && tm->data[4] != '1') ||
> - (tm->data[4] == '0' && tm->data[5] == '0') ||
> - (tm->data[4] == '1' && tm->data[5] > '2') ||
> - (tm->data[6] > '3') ||
> - (tm->data[6] == '0' && tm->data[7] == '0') ||
> - (tm->data[6] == '3' && tm->data[7] > '1') ||
> - (tm->data[8] > '2') ||
> - (tm->data[8] == '2' && tm->data[9] > '3') ||
> - (tm->data[10] > '5')) {
> + if ((data[4] != '0' && data[4] != '1') ||
> + (data[4] == '0' && data[5] == '0') ||
> + (data[4] == '1' && data[5] > '2') ||
> + (data[6] > '3') ||
> + (data[6] == '0' && data[7] == '0') ||
> + (data[6] == '3' && data[7] > '1') ||
> + (data[8] > '2') ||
> + (data[8] == '2' && data[9] > '3') ||
> + (data[10] > '5')) {
> LOG_DBG((LOG_POLICY, 30,
> "x509_generate_kn: invalid value in "
> "NotValidAfter time field"));
> goto fail;
> }
> - snprintf(after, sizeof after, "%s", tm->data);
> + snprintf(after, sizeof after, "%s", data);
> }
>
> /* Fix missing seconds. */
> - if (tm->length < 12) {
> + if (len < 12) {
> after[12] = '0';
> after[13] = '0';
> }
>
--
:wq Claudio
isakmpd: don't reach into ASN1_STRING [step 2/2]