CMS_final: do not ignore CMS_dataFinal result

Bug #1994165 reported by Gil Weis
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
openssl (Ubuntu)
Fix Released
Medium
Adrien Nader
Jammy
In Progress
Medium
Adrien Nader
Kinetic
Won't Fix
Medium
Adrien Nader
Lunar
Fix Released
Undecided
Unassigned

Bug Description

=== SRU information ===
[Meta]
This bug is part of a series of three bugs for a single SRU.
The "central" bug with the global information and debdiff is http://pad.lv/2033422

[Impact]
S/MIME signature can fail silently
The commit by upstream propagates the return code of some functions rather than ignore it.

[Test plan]
This issue is not very simple to reproduce because "openssl cms" cannot be used to do so. This has to be done with the openssl API instead.
At least the bug reportere here and the one on openssl's bug tracker have confirmed the patch solves the issue. Additionally, the bug reporter here has tested the PPA that contains the patche and validated it. Finally, I read through the patch attentively.

[Where problems could occur]
At this point it is unlikely an error would appear. The openssl bug tracker mentions nothing related to this patch which landed more than a year ago. The patch is simple and doesn't change the code logic.

[Patches]
The patches come directly from upstream and apply cleanly.

https://github.com/openssl/openssl/pull/18876

* https://git.launchpad.net/~adrien-n/ubuntu/+source/openssl/tree/debian/patches/jammy-sru-0001-REGRESSION-CMS_final-do-not-ignore-CMS_dataFinal-res.patch?h=jammy-sru&id=04ef023920ab08fba214817523fba897527dfff0
* https://git.launchpad.net/~adrien-n/ubuntu/+source/openssl/tree/debian/patches/jammy-sru-0002-Handle-SMIME_crlf_copy-return-code.patch?h=jammy-sru&id=04ef023920ab08fba214817523fba897527dfff0

=== Original description ===

https://github.com/openssl/openssl/pull/18876

The CMS_dataFinal result is important as signature may fail, however, it
is ignored while returning success from CMS_final.

Please add this fix to The openssl 3.0.2 "Jammy Jellyfish (supported)"

Thanks

Upstream commit:

```
commit 67c0460b89cc1b0644a1a59af78284dfd8d720af
Author: Alon Bar-Lev <email address hidden>
Date: Tue Jul 26 15:17:06 2022 +0300

    Handle SMIME_crlf_copy return code

    Currently the SMIME_crlf_copy result is ignored in all usages. It does
    return failure when memory allocation fails.

    This patch handles the SMIME_crlf_copy return code in all occurrences.

    Signed-off-by: Alon Bar-Lev <email address hidden>

    Reviewed-by: Tomas Mraz <email address hidden>
    Reviewed-by: Paul Dale <email address hidden>
    Reviewed-by: Hugo Landau <email address hidden>
    (Merged from https://github.com/openssl/openssl/pull/18876)
```

Revision history for this message
Benjamin Drung (bdrung) wrote :

"git tag --contains 67c0460b89cc1b0644a1a59af78284dfd8d720af" shows that no release contains the upstream commit yet.

description: updated
Changed in openssl (Ubuntu):
status: New → Triaged
importance: Undecided → Medium
Changed in openssl (Ubuntu Jammy):
status: New → Triaged
importance: Undecided → High
importance: High → Medium
Revision history for this message
Gil Weis (gilweis) wrote :

3.0.6 include this fix.

tags: added: foundations-triage-discuss
Revision history for this message
Lukas Märdian (slyon) wrote :

This should be fixed in lunar by merging openssl from Debian

Revision history for this message
Adrien Nader (adrien-n) wrote :

Hi Gil,

Can you explain a bit the actual impact of this bug and/or a scenario to reproduce. The commit doesn't give us a lot of details and the issue appears to be possibly quite serious but without diving deep into the code and possibly writing a reproducer from scratch ourselves, it is hard to be sure we properly understand it.

Thanks.

Revision history for this message
Gil Weis (gilweis) wrote : Re: [Bug 1994165] Re: CMS_final: do not ignore CMS_dataFinal result

Hi,
This is a serious bug.
CMS_final() finalises the structure cms. Its purpose is to perform any
operations necessary on cms.
CMS_final() call to SMIME_crlf_copy() and not checking the return value
from SMIME_crlf_copy() so even SMIME_crlf_copy() fail, CMS_final() will
return ok but with wrong CMS data.
SMIME_crlf_copy() copies data from in_bio to out_bio and it's used at the
final op on cms structure (for example before writing or sending cms object)
SMIME_crlf_copy will fail if some data in cms is missing or wrong.

Scenario to reproduce:
Create cms signature structure without the signature value and send it to
CMS_final(). CMS_final() will return ok even if the CMS_final() fails.
This causes the software to continue with incorrect information and pass it
on even though it is incorrect.

On Mon, Nov 14, 2022 at 5:40 PM Adrien Nader <email address hidden>
wrote:

> Hi Gil,
>
> Can you explain a bit the actual impact of this bug and/or a scenario to
> reproduce. The commit doesn't give us a lot of details and the issue
> appears to be possibly quite serious but without diving deep into the
> code and possibly writing a reproducer from scratch ourselves, it is
> hard to be sure we properly understand it.
>
> Thanks.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1994165
>
> Title:
> CMS_final: do not ignore CMS_dataFinal result
>
> Status in openssl package in Ubuntu:
> Triaged
> Status in openssl source package in Jammy:
> Triaged
> Status in openssl source package in Kinetic:
> Triaged
>
> Bug description:
> https://github.com/openssl/openssl/pull/18876
>
> The CMS_dataFinal result is important as signature may fail, however, it
> is ignored while returning success from CMS_final.
>
> Please add this fix to The openssl 3.0.2 "Jammy Jellyfish (supported)"
>
> Thanks
>
> Upstream commit:
>
> ```
> commit 67c0460b89cc1b0644a1a59af78284dfd8d720af
> Author: Alon Bar-Lev <email address hidden>
> Date: Tue Jul 26 15:17:06 2022 +0300
>
> Handle SMIME_crlf_copy return code
>
> Currently the SMIME_crlf_copy result is ignored in all usages. It
> does
> return failure when memory allocation fails.
>
> This patch handles the SMIME_crlf_copy return code in all
> occurrences.
>
> Signed-off-by: Alon Bar-Lev <email address hidden>
>
> Reviewed-by: Tomas Mraz <email address hidden>
> Reviewed-by: Paul Dale <email address hidden>
> Reviewed-by: Hugo Landau <email address hidden>
> (Merged from https://github.com/openssl/openssl/pull/18876)
> ```
>
> To manage notifications about this bug go to:
>
> https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1994165/+subscriptions
>
>

Revision history for this message
Gil Weis (gilweis) wrote :

Hi,
This is a serious bug.
CMS_final() finalises the structure cms. Its purpose is to perform any operations necessary on cms.
CMS_final() call to SMIME_crlf_copy() and not checking the return value from SMIME_crlf_copy() so even SMIME_crlf_copy() fail, CMS_final() will return ok but with wrong CMS data.
SMIME_crlf_copy() copies data from in_bio to out_bio and it's used at the final op on cms structure (for example before writing or sending cms object)
SMIME_crlf_copy will fail if some data in cms is missing or wrong.

Scenario to reproduce:
Create cms signature structure without the signature value and send it to CMS_final(). CMS_final() will return ok even if the CMS_final() fails.
This causes the software to continue with incorrect information and pass it on even though it is incorrect.

Revision history for this message
Adrien Nader (adrien-n) wrote :

We'd need more details about the issue and its actual impact for you since upstream doesn't consider this a security issue since it only happens when signing, not when checking signatures (which makes sense). Without this there is no process for pushing an update to a released version.

Adrien Nader (adrien-n)
Changed in openssl (Ubuntu Jammy):
assignee: nobody → Adrien Nader (adrien-n)
Changed in openssl (Ubuntu Kinetic):
assignee: nobody → Adrien Nader (adrien-n)
Changed in openssl (Ubuntu):
assignee: nobody → Adrien Nader (adrien-n)
Changed in openssl (Ubuntu Jammy):
status: Triaged → Incomplete
Changed in openssl (Ubuntu Kinetic):
status: Triaged → Incomplete
Changed in openssl (Ubuntu):
status: Triaged → Incomplete
Revision history for this message
Gil Weis (gilweis) wrote :

Thanks.

Revision history for this message
Adrien Nader (adrien-n) wrote :

I was closing this for the reasons I outlined above. However, since then, I've decided to try to do an SRU of openssl for Jammy and I can try to integrate these changes.

Changed in openssl (Ubuntu):
status: Incomplete → Won't Fix
Changed in openssl (Ubuntu Jammy):
status: Incomplete → Won't Fix
Changed in openssl (Ubuntu Kinetic):
status: Incomplete → Won't Fix
Changed in openssl (Ubuntu):
status: Won't Fix → Triaged
Changed in openssl (Ubuntu Jammy):
status: Won't Fix → Triaged
Adrien Nader (adrien-n)
Changed in openssl (Ubuntu Jammy):
status: Triaged → In Progress
milestone: none → jammy-updates
Changed in openssl (Ubuntu):
status: Triaged → In Progress
Adrien Nader (adrien-n)
Changed in openssl (Ubuntu Lunar):
status: New → Fix Released
Changed in openssl (Ubuntu):
status: In Progress → Fix Released
Revision history for this message
Adrien Nader (adrien-n) wrote :

I've created a PPA for Jammy that incorporates the fix mentionned. The details are available at https://launchpad.net/~adrien-n/+archive/ubuntu/openssl-jammy-sru . Could you test it and confirm your issue is solved?

Revision history for this message
Gil Weis (gilweis) wrote :

Hi
It seems that the issue is solved.
Thanks

On Tue, Sep 12, 2023 at 12:16 PM Adrien Nader <email address hidden>
wrote:

> I've created a PPA for Jammy that incorporates the fix mentionned. The
> details are available at
> https://launchpad.net/~adrien-n/+archive/ubuntu/openssl-jammy-sru .
> Could you test it and confirm your issue is solved?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1994165
>
> Title:
> CMS_final: do not ignore CMS_dataFinal result
>
> Status in openssl package in Ubuntu:
> Fix Released
> Status in openssl source package in Jammy:
> In Progress
> Status in openssl source package in Kinetic:
> Won't Fix
> Status in openssl source package in Lunar:
> Fix Released
>
> Bug description:
> https://github.com/openssl/openssl/pull/18876
>
> The CMS_dataFinal result is important as signature may fail, however, it
> is ignored while returning success from CMS_final.
>
> Please add this fix to The openssl 3.0.2 "Jammy Jellyfish (supported)"
>
> Thanks
>
> Upstream commit:
>
> ```
> commit 67c0460b89cc1b0644a1a59af78284dfd8d720af
> Author: Alon Bar-Lev <email address hidden>
> Date: Tue Jul 26 15:17:06 2022 +0300
>
> Handle SMIME_crlf_copy return code
>
> Currently the SMIME_crlf_copy result is ignored in all usages. It
> does
> return failure when memory allocation fails.
>
> This patch handles the SMIME_crlf_copy return code in all
> occurrences.
>
> Signed-off-by: Alon Bar-Lev <email address hidden>
>
> Reviewed-by: Tomas Mraz <email address hidden>
> Reviewed-by: Paul Dale <email address hidden>
> Reviewed-by: Hugo Landau <email address hidden>
> (Merged from https://github.com/openssl/openssl/pull/18876)
> ```
>
> To manage notifications about this bug go to:
>
> https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/1994165/+subscriptions
>
>

Revision history for this message
Adrien Nader (adrien-n) wrote :

Thanks a lot for taking the time to test and provide feedback.

Adrien Nader (adrien-n)
description: updated
Adrien Nader (adrien-n)
description: updated
Adrien Nader (adrien-n)
description: updated
Adrien Nader (adrien-n)
description: updated
description: updated
Adrien Nader (adrien-n)
description: updated
Adrien Nader (adrien-n)
description: updated
Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Hello,

ubuntu-sponsors is subscribed to this bug but I couldn't find anything actionable. I'm unsubscribing ubuntu-sponsors; feel free to subscribe it again if there's anything that needs sponsoring. Thanks.

Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

Ah, I noticed that this is part of a big SRU that's being completed on bug #2033422. Just leaving a comment here for the record.

Revision history for this message
Simon Chopin (schopin) wrote :

A version containing a fix for this has been uploaded to the Jammy queue to be processed by the SRU team. Thanks, Adrien :)

Adrien Nader (adrien-n)
description: updated
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.