SPKAC functionality per feature request #38917 (part deux) #37

Closed
wants to merge 5 commits into from

3 participants

@jas-

Per bug id https://bugs.php.net/bug.php?id=38917, a patch to implement missing SPKAC functionality within the OpenSSL extension.

I have made recommended modifications per closed pull request id #21.

@smalyshev smalyshev and 2 others commented on an outdated diff Apr 4, 2012
ext/openssl/openssl.c
+
+ if (!NETSCAPE_SPKI_sign(spki, pkey, mdtype)) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to sign with %l algorithm", method);
+ RETURN_NULL();
+ }
+
+ spkstr = NETSCAPE_SPKI_b64_encode(spki);
+ if (!spkstr){
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable encode SPKAC");
+ RETURN_NULL();
+ }
+
+ s = emalloc(strlen(spkac) + strlen(spkstr) + 1);
+ sprintf(s, "%s%s", spkac, spkstr);
+
+ if (sizeof(s)<=0) {

what this code is for? emalloc never returns NULL and sizeof(s) is a constant and you used s for sprintf anyway earlier.

@jas-
jas- added a note Apr 4, 2012

Perhaps I don't understand the question. sprintf() gets used after emalloc

What is this code for: if (sizeof(s)<=0) {

@jas-
jas- added a note Apr 4, 2012

To ensure an SPKAC is created, I should modify that to be

if (sizeof(s)!=8) {
@nikic
nikic added a note Apr 4, 2012

@jas- In which occasions would it not be created? As @smalyshev mentioned, emalloc is infallible. If it can't allocate the memory the program will terminate.

@jas-
jas- added a note Apr 4, 2012

Didn't think of that, over engineering it. WIll remove thanks @smalyshev & @nikic

Also, sizeof(s) is always the same, it is a constant operation. It does not depend on anything, just returns size of char * type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev and 1 other commented on an outdated diff Apr 4, 2012
ext/openssl/openssl.c
+ s = emalloc(strlen(spkac) + strlen(spkstr) + 1);
+ sprintf(s, "%s%s", spkac, spkstr);
+
+ if (sizeof(s)<=0) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to allocate memory for SPKAC");
+ RETURN_NULL();
+ }
+
+ if (keyresource == -1 && spki != NULL) {
+ NETSCAPE_SPKI_free(spki);
+ }
+ if (keyresource == -1 && pkey != NULL) {
+ EVP_PKEY_free(pkey);
+ }
+
+ RETURN_STRINGL(s, strlen(s), 1);

Why s is allocated and then copied? Can't you avoid the copy and return s as is? Also, s is never freed.

@jas-
jas- added a note Apr 4, 2012

I suppose because the remainder of the functions in the openssl.c file contain similar methods of allocating memory for pointers prior to copying contents although most of them deal with the BIO_* family of functions provided by the ssl libs.

According to the documentation on the RETURN_STRINGL() macro it will actually perform the free/efree operations unless I am missing something

I don't know what documentation you are referring to, I don't see RETURN_STRINGL doing any free or efree. What documentation says it does?

@jas-
jas- added a note Apr 4, 2012

I am mistaken. The last argument was to copy not free the string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev and 1 other commented on an outdated diff Apr 4, 2012
ext/openssl/openssl.c
+ openssl_spki_cleanup(spkstr, spkstr_cleaned);
+
+ if (strlen(spkstr_cleaned)<=0) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to allocate memory for SPKAC");
+ RETURN_FALSE;
+ }
+
+ spki = NETSCAPE_SPKI_b64_decode(spkstr_cleaned, strlen(spkstr_cleaned));
+ if (spki == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to decode supplied SPKAC");
+ RETURN_FALSE;
+ }
+
+ pkey = X509_PUBKEY_get(spki->spkac->pubkey);
+ if (pkey == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to aquire signed public key");

Wouldn't this leak spki?

@jas-
jas- added a note May 30, 2012

Fix is on the way, just performing tests to ensure leaks are no longer applicable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev and 1 other commented on an outdated diff Apr 4, 2012
ext/openssl/openssl.c
+ if (spki == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to decode supplied SPKAC");
+ RETURN_NULL();
+ }
+
+ pkey = X509_PUBKEY_get(spki->spkac->pubkey);
+ if (pkey == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to aquire signed public key");
+ RETURN_NULL();
+ }
+
+ PEM_write_bio_PUBKEY(out, pkey);
+ BIO_get_mem_ptr(out, &bio_buf);
+
+ if ((!bio_buf->data)&&(bio_buf->length<=0)) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to allocate memory for public key");

here we would leak pkey, spki, spkstr_cleaned

@jas-
jas- added a note Apr 4, 2012

So would you rather it worked like this?

PEM_write_bio(out, X509_PUBKEY_get(spki->spkac->pubkey));

PEM_write_bio call doesn't matter, what matters you are returning without freeing structures that you have allocated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev and 1 other commented on an outdated diff Apr 4, 2012
ext/openssl/openssl.c
+ }
+
+ s = emalloc(bio_buf->length);
+ BIO_read(out, s, bio_buf->length);
+
+ if (spki != NULL) {
+ NETSCAPE_SPKI_free(spki);
+ }
+ if (out != NULL) {
+ BIO_free_all(out);
+ }
+ if (pkey != NULL) {
+ EVP_PKEY_free(pkey);
+ }
+
+ RETURN_STRINGL(s, strlen(s), 1);

Is s and spkstr_cleaned ever freed?

@jas-
jas- added a note Apr 4, 2012

Apparently not but upon review of other functions in the openssl.c file there are resources that are similar, openssl_x509_parse() for example doesn't free a few items. Could you be specific in what the problem is?

The problem is that you allocate s but do not free it. It creates memory leak. It should be fixed.

@jas-
jas- added a note Apr 5, 2012

No worries, my dyslexia rears its ugly head. I misread the docs on RETURN_STRINGL. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev and 1 other commented on an outdated diff Apr 4, 2012
ext/openssl/openssl.c
+
+ if (spkstr == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to use supplied SPKAC");
+ RETURN_NULL();
+ }
+
+ spkstr_cleaned = emalloc(spkstr_len + 1);
+ openssl_spki_cleanup(spkstr, spkstr_cleaned);
+
+ spki = NETSCAPE_SPKI_b64_decode(spkstr_cleaned, strlen(spkstr_cleaned));
+ if (spki == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "unable to allocate memory for public key");
+ RETURN_NULL();
+ }
+
+ RETURN_STRINGL(ASN1_STRING_data(spki->spkac->challenge), strlen(ASN1_STRING_data(spki->spkac->challenge)), 1);

Is spkstr_cleaned ever freed?

@jas-
jas- added a note Apr 6, 2012

@smalyshev Could you confirm that the document I am referencing here (http://docstore.mik.ua/orelly/webprog/php/ch14_08.htm) is correct?

PHP_FUNCTION(test) {
    char *str = emalloc(5);
    strcpy(str, "newb");
    RETURN_STRINGL(str, strlen(str), 0);
}

By setting the last argument of RETURN_STRINGL to 0 anything emalloc'd will get efree'd internally or no?

The last argument of RETURN_STRING[L] says if the argument string should be duplicated or not. If the string is already allocated with emalloc (IMPORTANT: not malloc!) then you can pass 0 zero and you don't have to free the string and there's no duplication.

If you pass 1, the string will be duplicated and if you allocated the original you have the responsibility to free it.

If you use strlen, recommended way would be to do RETURN_STRING(str, 0); as RETURN_STRING automatically says length to strlen of the string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev commented on the diff May 24, 2012
ext/openssl/openssl.c
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unknown signature algorithm");
+ RETURN_NULL();
+ }
+
+ if ((spki = NETSCAPE_SPKI_new()) == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to create new SPKAC");
+ RETURN_NULL();
+ }
+
+ if (challenge) {
+ ASN1_STRING_set(spki->spkac->challenge, challenge, (int)strlen(challenge));
+ }
+
+ if (!NETSCAPE_SPKI_set_pubkey(spki, pkey)) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to embed public key");
+ RETURN_NULL();

Wouldn't this RETURN leave spki and pkey not freed?

@jas-
jas- added a note May 24, 2012

I would much rather use a macro as I did in my original patch submission to free everything.

if (!NETSCAPE_SPKI_set_pubkey(spki, pkey)) {
    goto cleanup;

I was however told I should move away from using those

You can use macro, you can use goto if you're really careful (comment on it!) but you have to free stuff you allocated one way or another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev commented on the diff May 24, 2012
ext/openssl/openssl.c
+
+ if (strlen(spkstr_cleaned)<=0) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to allocate memory for SPKAC");
+ RETURN_FALSE;
+ }
+
+ spki = NETSCAPE_SPKI_b64_decode(spkstr_cleaned, strlen(spkstr_cleaned));
+ if (spki == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to decode supplied SPKAC");
+ RETURN_FALSE;
+ }
+
+ pkey = X509_PUBKEY_get(spki->spkac->pubkey);
+ if (pkey == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to aquire signed public key");
+ RETURN_FALSE;

This RETURN seems not to free all temp structured previously allocated.

@jas-
jas- added a note May 24, 2012

Again, move to a macro to perform all free'ing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev commented on the diff May 24, 2012
ext/openssl/openssl.c
+ openssl_spki_cleanup(spkstr, spkstr_cleaned);
+
+ spki = NETSCAPE_SPKI_b64_decode(spkstr_cleaned, strlen(spkstr_cleaned));
+ if (spki == NULL) {
+ php_error_docref(NULL TSRMLS_CC, E_WARNING, "Unable to allocate memory for public key");
+ RETURN_NULL();
+ }
+ /*
+ if (spkstr != NULL) {
+ efree(spkstr);
+ }
+ if (spkstr_cleaned != NULL) {
+ efree(spkstr_cleaned);
+ }
+ */
+ RETURN_STRING(ASN1_STRING_data(spki->spkac->challenge), 0);

aren't you supposed to free spki data structure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev smalyshev commented on the diff May 24, 2012
ext/openssl/tests/openssl_spki_export_challenge.phpt
+echo ($pkey !== false) ?
+ "ok!\n\n" : "error creating private key\n\n";
+
+/* testing all signing algorithms */
+$algos = array('MD5'=>OPENSSL_ALGO_MD5,
+ 'SHA1'=>OPENSSL_ALGO_SHA1,
+ 'SHA256'=>OPENSSL_ALGO_SHA256,
+ 'SHA512'=>OPENSSL_ALGO_SHA512);
+
+/* loop to create, export challenge and verify */
+foreach($algos as $key => $value){
+ echo "Exporting challenge from SPKAC with ".$key."...";
+ $spki = openssl_spki_new($pkey, "sample_challenge_string", $value);
+ if ((!empty($spki)) && (preg_match('/SPKAC=[a-zA-Z0-9\/\+]/', $spki)))
+ $r = openssl_spki_export_challenge(preg_replace('/SPKAC=/', '', $spki));
+ if ($r === "sample_challenge_string")

I think it is better to do var_dump($r) here and then have the string in the EXPECT. This way if $r is something else it is immediately seen in the test diff.

@jas-
jas- added a note May 24, 2012

Noted. I will make adjustments hopefully over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lstrojny

@jas- ping again

@jas-

Closing this request. Please refer to branch "issue-38917-spkac" and pull request #267 (#267)

@jas- jas- closed this Jan 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment