Clear outputs in PKCS12_parse error handling

[bernd-edlinger]

The function PKCS12_parse may return invalid pointers in *pkey and *cert
in case of an error.
This ensures that the returned values are always zero if anything happens.

[dot-asm]

This calls for kind of "philosophical" question. Is it appropriate to modify any of **arguments if you end up returning error? Maybe it would be more appropriate to keep results in local variables till you know that you'll be returning success and only then pass them back through corresponding *arguments? On the other hand if zeroing *arguments is deemed appropriate, then perhaps it would be worth mentioning in "RETURN VALUES" paragraph?

Comment applies even to #4146.

[bernd-edlinger]

Don't know. For me it is more a practical problem, how to write a correct application.

The problem is that ca is an in-out and whatever is there on return needs
to be cleaned up in the outer error handling. And the others are out only.

But pkey and cert need not even be initialized to zero before the call.
So the only "correct" way to do the error handling would be something
like that:

pkey = NULL;
cert = NULL;
ca = NULL;
if (PKCS12_parse(p12, "pass", &pkey, &cert, &ca) <= 0) {
  evp = NULL; cert = NULL;
}
else {
  do_something(pkey, cert, ca);
}
EVP_PKEY_free(pkey);
X509_free(cert);
sk_X509_pop_free(ca, X509_free);

I expect that mostly all programs will look like that:

ca = NULL;
if (PKCS12_parse(p12, "pass", &pkey, &cert, &ca) > 0) {
  do_something(pkey, cert, ca);
}
EVP_PKEY_free(pkey);
X509_free(cert);
sk_X509_pop_free(ca, X509_free);

And that's what I would like to fix.

[dot-asm]

I'd say that it would rather be

ca = NULL;
if (PKCS12_parse(p12, "pass", &pkey, &cert, &ca) > 0) {
  do_something(pkey, cert, ca);
  EVP_PKEY_free(pkey);
  X509_free(cert);
  sk_X509_pop_free(ca, X509_free);
}

But the original question is rather about formal style. You can view it like this. Which of the two would make auditor's life easier. Not necessarily human's, one can as well wonder how would it affect resource consumption of some kind of static code analyzer...

[dot-asm]

Update to .pod means that you might have to split request even between 1.1.0 and master. As alternative it might be appropriate to say that code modifications apply to both, but .pod update, only to master. And if deemed appropriate, .pod updated for 1.1.0 can be handled as separate request...

[bernd-edlinger]

Yes, I'd expect the code change can be cherry picked to 1.1.0 and the .pod update is master only.

[bernd-edlinger]

In your code example there is a potential memory leak in *ca, which is not and has never
been cleaned up in the error handling. This PR does not touch the *ca resource-flow.

Returning *ca in exactly the same state as it was before the call could be challenging.
That's definitely something for another PR, but probably not from me.

[dot-asm]

In your code example there is a potential memory leak in *ca.

Maybe, but it has lesser to do with the original point:-) Suggested modification is fine for current semantic, but my question if we need to change the latter. My suggestion to wait a little bit for others to speak up...

[dot-asm]

I'm adding "pending 2nd review" to denote the fact that additional opinion is requested. Question is what is more appropriate. To modify output arguments even if en error is returned (as currently). Or to modify the code so that in case of error output arguments are never modified.

[paulidale]

I'd go for the don't change any of the output arguments on error option.

For pkey and cert this is easy. For ca it would be more difficult but is possible. A count of the pushed items and a loop to pop then on error. The structure could have moved due to the realloc so there is a potential (harmless) change still. To achieve no change, a new stack API to reserve space would be required. Essentially guaranteeing the next however many push operations won't fail due to memory concerns.

[t-j-h]

From a pragmatic point of view the normal style I would follow is that you clear your output arguments (if they are pointers to pointers set them to NULL) so that they are "valid" on return whether there is an error or not. Guaranteeing not to change output arguments on error is something I would absolutely not do - as there are a large range of code paths in general and either it is okay to start changing values or you should never change values. The half-way-in-between doesn't help anyone.

[paulidale]

The half-way-in-between doesn't help anyone.

Definitely.

[kaduk]

As an additional data point, IIUC in krb5 style we would do something like this pseudocode:

PKCS12_parse(..., STACK_OF(X509) **ca_out)
{
    STACK_OF(X509) *ca = sk_X509_dup(*ca_out)
   [...]
   if (error) {
       sk_X509_pop_free(ca, X509_free);
    } else {
        sk_X509_pop_free(ca_out, X509_free);
        *ca_out = ca;
    }

which uses more memory but is easier to reason about.
I think in general one can make reasonable cases for "free/set to NULL on error" and for "don't touch anything on error", but in this case the existing API w.r.t ca is clunky enough that trying to bolt the former scheme on top of it would not work well. That is, I am leaning towards "don't touch anythign at all on error".

[paulidale]

That's a much easier way to handle ca.

I think it is the input/output ca argument that is the main concern here. Should the function free and NULL the user's input on error? That seems wrong to me.

I'm kind of tempted to add a reserve n new slots to the stack data type -- pretty much all the code for this is present already.

[bernd-edlinger]

I would expect that keeping whatever is in pkey and cert before the call can easily break
existing applications, because they might rely on PKCS12_parse to overwrite pkey and cert
right in the beginning. They might be uninitialized stack values for instance.

All I want to change here is a corner-case that is hard to reach without either a out-of-memory
error in the sk_push, or with a especially crafted .p12 file that has correct MAC and
correct PKEY but a syntax error in the last bag.

I agree that keeping ca in exactly the same state as before the call might be an improvement.
But I think that would be another PR.

[FdaSilvaYY]

@paulidale: I already write this code to pre-allocate stack ; it is staging in my repo. I can PR it ?

[dot-asm]

I would expect that keeping whatever is in pkey and cert before the call can easily break
existing applications, because they might rely on PKCS12_parse to overwrite pkey and cert
right in the beginning.

It would be utterly irresponsible of them to make such assumption in case an error is returned. Besides, suggested fix actually speaks against this argument. Because as it is now it frees *pkey, but doesn't nullifies it, effectively exposing caller that wouldn't pay attention to double-free. According to your reasoning that is. Or in other words if there were applications that didn't pay attention to return code, then we would probably know about it.

Well, if you examine call to PKCS12_parse in apps/apps.c you might get an impression that suggested nullifying would be appropriate. But at the same time not touching arguments would work (and I'd still argue that it's more appropriate), because caller is explicit about nullifying before call.

"Vote count" appears to be 2 vs. 3 in favour of don't touch in case of error, or did I count wrong?

[bernd-edlinger]

Because as it is now it frees *pkey, but doesn't nullifies it, effectively exposing caller that wouldn't pay attention to double-free. According to your reasoning that is. Or in other words if there were applications that didn't pay attention to return code, then we would probably know about it.

Sorry I can't agree with that. It should be extremely difficult to reach that code path where *pkey
is freed in the error handling. I have only done a code review, but did never see this actually happen.

[bernd-edlinger]

By the way, when I look at apps/apps.c, there is one call that will probably never work:

    } else if (format == FORMAT_PKCS12) {
        if (!load_pkcs12(cert, cert_descrip, NULL, NULL, NULL, &x, NULL))
            goto end;

That is because PKCS12_parse will never populate *cert if pkey is NULL.
All certificates go into *ca but ca is also NULL, so nothing happens.
That might be something that we could also want to make work ?

[dot-asm]

It should be extremely difficult to reach that code path where *pkey is freed in the error handling.

Does it matter in the context? I mean formally speaking it is still a possibility... Well, you can argue that not nullifying was a bug and it's OK for application to disregard return code and instead rely on values *pkey and *cert being NULL. Note however that manual page reads "if successful the private key will be written to *pkey, ..."

[dot-asm]

apps/apps.c, ... make work ?

Sure, it would be appropriate. Directly here or elsewhere...

[kaduk]

I think I have lost rack of what the next step is, here. (I would not mind if the current diff gets applied as an interim step; it seems a clear improvement over the current master.)

[paulidale]

@paulidale: I already write this code to pre-allocate stack ; it is staging in my repo. I can PR it ?

I've no objection, but it might be worthwhile waiting to see the answer to the erase or don't touch question.

[dot-asm]

I think I have lost rack of what the next step is

I reckon that majority of involved is in favour of not touching output arguments in case of error. One can argue that it would be sensible to commit this and address not-touching in another request, but then somebody has to keep track. One can put a note in form of issue, but in such case I would suggest to abstain from modifying corresponding .pod file in this request.

[bernd-edlinger]

Yes. I agree. Especially for master, this sounds reasonable.
Could an OMC-member please approve the first commit in this PR and #4146 then?

[dot-asm]

1st commit only

[richsalz]

like andy said

[bernd-edlinger]

Pushed first commit to master and 1.1.0, thanks!