On 2017-12-17 18:35:17 [+0100], Hilko Bengen wrote:
> Control: tag -1 patch -fixed-upstream
>
> I don't see where the direct struct access issues have been fixed
> upstream, the source code snapshot available from
> http://www.kermitproject.org/ckdaily.html still has lots of those.
>
> I have prepared a patch that fixes building with OpenSSL 1.1 and would
> appreciate a review.
>Index: ckermit-302/ck_ssl.c
>===================================================================
>--- ckermit-302.orig/ck_ssl.c
>+++ ckermit-302/ck_ssl.c
>@@ -975,13 +981,15 @@ static DH *
> get_dh1536()
> {
> DH *dh=NULL;
>+ BIGNUM *p, *g;
>
> if ((dh=DH_new()) == NULL)
> return(NULL);
>- dh->p=BN_bin2bn(dh1536_p,sizeof(dh1536_p),NULL);
>- dh->g=BN_bin2bn(dh1536_g,sizeof(dh1536_g),NULL);
>- if ((dh->p == NULL) || (dh->g == NULL))
>+ p=BN_bin2bn(dh1536_p,sizeof(dh1536_p),NULL);
>+ g=BN_bin2bn(dh1536_g,sizeof(dh1536_g),NULL);
>+ if ((p == NULL) || (g == NULL))
> return(NULL);
>+ DH_set0_pqg(dh, p, NULL, g);
> return(dh);
Those DH values should not be static / compiled in (e.g. the same on
every server) but not the scope of the patch…
> }
>
>@@ -1457,13 +1468,15 @@ the build.\r\n\r\n");
>
> #ifdef ZLIB
> cm = COMP_zlib();
>- if (cm != NULL && cm->type != NID_undef) {
>+ if (cm != NULL && SSL_COMP_get_id(cm) != NID_undef) {
> SSL_COMP_add_compression_method(0xe0, cm); /* EAY's ZLIB ID */
> }
> #endif /* ZLIB */
>+#if 0 /* COMP_rle has apparently been removed in OpenSSL 1.1 */
> cm = COMP_rle();
> if (cm != NULL && cm->type != NID_undef)
> SSL_COMP_add_compression_method(0xe1, cm); /* EAY's RLE ID */
>+#endif
yes, but that ZLIB in the above shouldn't work since it should not be
compiled in :) Anyway, a check for OpenSSL version instead that if0
would be better.
>@@ -2583,7 +2598,7 @@ ssl_anonymous_cipher(ssl) SSL * ssl;
> int
> ssl_verify_crl(int ok, X509_STORE_CTX *ctx)
> {
>- X509_OBJECT obj;
>+ X509_OBJECT *obj = X509_OBJECT_new();
this may be NULL
> X509_NAME *subject = NULL;
> X509_NAME *issuer = NULL;
> X509 *xs = NULL;
>@@ -2649,11 +2664,10 @@ ssl_verify_crl(int ok, X509_STORE_CTX *c
> * Try to retrieve a CRL corresponding to the _subject_ of
> * the current certificate in order to verify it's integrity.
> */
and before that where were two return statemens where you would leak
`obj'.
>- memset((char *)&obj, 0, sizeof(obj));
> X509_STORE_CTX_init(store_ctx, crl_store, NULL, NULL);
>- rc = X509_STORE_get_by_subject(store_ctx, X509_LU_CRL, subject, &obj);
>+ rc = X509_STORE_get_by_subject(store_ctx, X509_LU_CRL, subject, obj);
> X509_STORE_CTX_cleanup(store_ctx);
>- crl = obj.data.crl;
>+ crl = X509_OBJECT_get0_X509_CRL(obj);
> if (rc > 0 && crl != NULL) {
> /*
> * Verify the signature on this CRL
>@@ -2661,7 +2675,7 @@ ssl_verify_crl(int ok, X509_STORE_CTX *c
> if (X509_CRL_verify(crl, X509_get_pubkey(xs)) <= 0) {
> fprintf(stderr, "Invalid signature on CRL!\n");
> X509_STORE_CTX_set_error(ctx, X509_V_ERR_CRL_SIGNATURE_FAILURE);
>- X509_OBJECT_free_contents(&obj);
>+ X509_OBJECT_free(obj);
> X509_STORE_CTX_free(store_ctx);
> return 0;
> }
>@@ -2674,7 +2688,7 @@ ssl_verify_crl(int ok, X509_STORE_CTX *c
> fprintf(stderr, "Found CRL has invalid nextUpdate field.\n");
> X509_STORE_CTX_set_error(ctx,
>
> X509_V_ERR_ERROR_IN_CRL_NEXT_UPDATE_FIELD);
>- X509_OBJECT_free_contents(&obj);
>+ X509_OBJECT_free(&obj);
> X509_STORE_CTX_free(store_ctx);
> return 0;
> }
>@@ -2683,11 +2697,11 @@ ssl_verify_crl(int ok, X509_STORE_CTX *c
> "Found CRL is expired - revoking all certificates until you get updated
> CRL.\n"
> );
> X509_STORE_CTX_set_error(ctx, X509_V_ERR_CRL_HAS_EXPIRED);
>- X509_OBJECT_free_contents(&obj);
>+ X509_OBJECT_free(&obj);
> X509_STORE_CTX_free(store_ctx);
> return 0;
> }
>- X509_OBJECT_free_contents(&obj);
>+ X509_OBJECT_free(&obj);
I *think* that this won't work. You free `obj' here and in the next hunk
you use it again.
> }
>
> /*
>@@ -2698,7 +2712,7 @@ ssl_verify_crl(int ok, X509_STORE_CTX *c
> X509_STORE_CTX_init(store_ctx, crl_store, NULL, NULL);
> rc = X509_STORE_get_by_subject(store_ctx, X509_LU_CRL, issuer, &obj);
> X509_STORE_CTX_free(store_ctx); /* calls
> X509_STORE_CTX_cleanup() */
>- crl = obj.data.crl;
>+ crl = X509_OBJECT_get0_X509_CRL(obj);
> if (rc > 0 && crl != NULL) {
> /*
> * Check if the current certificate is revoked by this CRL
>@@ -2706,19 +2720,19 @@ ssl_verify_crl(int ok, X509_STORE_CTX *c
> n = sk_X509_REVOKED_num(X509_CRL_get_REVOKED(crl));
> for (i = 0; i < n; i++) {
> revoked = sk_X509_REVOKED_value(X509_CRL_get_REVOKED(crl), i);
>- if (ASN1_INTEGER_cmp(revoked->serialNumber,
>+ if (ASN1_INTEGER_cmp(X509_REVOKED_get0_serialNumber(revoked),
> X509_get_serialNumber(xs)) == 0) {
>
>- serial = ASN1_INTEGER_get(revoked->serialNumber);
>+ serial =
>ASN1_INTEGER_get(X509_REVOKED_get0_serialNumber(revoked));
> cp = X509_NAME_oneline(issuer, NULL, 0);
> free(cp);
>
> X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_REVOKED);
>- X509_OBJECT_free_contents(&obj);
>+ X509_OBJECT_free(&obj);
> return 0;
> }
> }
>- X509_OBJECT_free_contents(&obj);
>+ X509_OBJECT_free(&obj);
> }
I would move that out of that loop/condition since you want to free that
in every case.
> return ok;
> }
I am impressed by the amount of SSL usage for a terminal program.
> Cheers,
> -Hilko
Sebastian
