| Message ID | 20230505111814.491483-1-andrej.valek@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | cve-check: add option to add additional patched CVEs | expand |
On Fri, 2023-05-05 at 13:18 +0200, Andrej Valek via lists.openembedded.org wrote: > CVE_CHECK_PATCHED - should contains an additional CVEs which have been > fixed and shouldn't be mark as vulnerable nor ignored. > > Signed-off-by: Andrej Valek <andrej.valek@siemens.com> > --- > meta/classes/cve-check.bbclass | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve-check.bbclass > index bd9e7e7445c..957ea0130dc 100644 > --- a/meta/classes/cve-check.bbclass > +++ b/meta/classes/cve-check.bbclass > @@ -78,6 +78,11 @@ CVE_CHECK_SKIP_RECIPE ?= "" > # > CVE_CHECK_IGNORE ?= "" > > +# Usually a CVE gets treated as patched when a patch with the name of the CVE > +# gets applied. Basically this variable should not be used. But if there are > +# other reasons to mark a CVE as patched it can be added to this list. > +CVE_CHECK_PATCHED ?= "" We're not adding variables which are documented as "Basically this variable should not be used.". If you shouldn't need/use it, we don't need it. Can't you just use the ignore variable for the same end result? Cheers, Richard
On Fri, 2023-05-05 at 12:30 +0100, Richard Purdie wrote: > On Fri, 2023-05-05 at 13:18 +0200, Andrej Valek via > lists.openembedded.org wrote: > > CVE_CHECK_PATCHED - should contains an additional CVEs which have > > been > > fixed and shouldn't be mark as vulnerable nor ignored. > > > > Signed-off-by: Andrej Valek <andrej.valek@siemens.com> > > --- > > meta/classes/cve-check.bbclass | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve- > > check.bbclass > > index bd9e7e7445c..957ea0130dc 100644 > > --- a/meta/classes/cve-check.bbclass > > +++ b/meta/classes/cve-check.bbclass > > @@ -78,6 +78,11 @@ CVE_CHECK_SKIP_RECIPE ?= "" > > # > > CVE_CHECK_IGNORE ?= "" > > > > +# Usually a CVE gets treated as patched when a patch with the name > > of the CVE > > +# gets applied. Basically this variable should not be used. But if > > there are > > +# other reasons to mark a CVE as patched it can be added to this > > list. > > +CVE_CHECK_PATCHED ?= "" > > We're not adding variables which are documented as "Basically this > variable should not be used.". If you shouldn't need/use it, we don't > need it. Ok, maybe I should change the description a little bit. Do you have some other preference? > > Can't you just use the ignore variable for the same end result? Nope. If I use a ignore list, the output in the SBOM will be set to "ignored", which is wrong, because it has been fixed. And that's the reason. > > Cheers, > > Richard > Regards, Andrej
On Fri, 2023-05-05 at 11:36 +0000, Valek, Andrej wrote: > On Fri, 2023-05-05 at 12:30 +0100, Richard Purdie wrote: > > On Fri, 2023-05-05 at 13:18 +0200, Andrej Valek via > > lists.openembedded.org wrote: > > > CVE_CHECK_PATCHED - should contains an additional CVEs which have > > > been > > > fixed and shouldn't be mark as vulnerable nor ignored. > > > > > > Signed-off-by: Andrej Valek <andrej.valek@siemens.com> > > > --- > > > meta/classes/cve-check.bbclass | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve- > > > check.bbclass > > > index bd9e7e7445c..957ea0130dc 100644 > > > --- a/meta/classes/cve-check.bbclass > > > +++ b/meta/classes/cve-check.bbclass > > > @@ -78,6 +78,11 @@ CVE_CHECK_SKIP_RECIPE ?= "" > > > # > > > CVE_CHECK_IGNORE ?= "" > > > > > > +# Usually a CVE gets treated as patched when a patch with the name > > > of the CVE > > > +# gets applied. Basically this variable should not be used. But if > > > there are > > > +# other reasons to mark a CVE as patched it can be added to this > > > list. > > > +CVE_CHECK_PATCHED ?= "" > > > > We're not adding variables which are documented as "Basically this > > variable should not be used.". If you shouldn't need/use it, we don't > > need it. > Ok, maybe I should change the description a little bit. Do you have > some other preference? > > > > Can't you just use the ignore variable for the same end result? > Nope. If I use a ignore list, the output in the SBOM will be set to > "ignored", which is wrong, because it has been fixed. And that's the > reason. > I suspect "ignored" is a bad way to describe things. Ignore might mean the issue doesn't apply, has been fixed in some way or we really are ignoring it. What does the SBOM spec say about different field values? Should we be providing more reasoning than just adding to an ignore list? I'm a bit worried we're not solving the real problem here by adding a new variable we tell people not to use. Cheers, Richard
On Fri, 2023-05-05 at 12:59 +0100, Richard Purdie wrote: > > On Fri, 2023-05-05 at 11:36 +0000, Valek, Andrej wrote: > > > > On Fri, 2023-05-05 at 12:30 +0100, Richard Purdie wrote: > > > > > > On Fri, 2023-05-05 at 13:18 +0200, Andrej Valek via > > > > > > lists.openembedded.org wrote: > > > > > > > > CVE_CHECK_PATCHED - should contains an additional CVEs > > > > > > > > which > > > > > > > > have > > > > > > > > been > > > > > > > > fixed and shouldn't be mark as vulnerable nor ignored. > > > > > > > > > > > > > > > > Signed-off-by: Andrej Valek <andrej.valek@siemens.com> > > > > > > > > --- > > > > > > > > meta/classes/cve-check.bbclass | 8 ++++++++ > > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > > > diff --git a/meta/classes/cve-check.bbclass > > > > > > > > b/meta/classes/cve- > > > > > > > > check.bbclass > > > > > > > > index bd9e7e7445c..957ea0130dc 100644 > > > > > > > > --- a/meta/classes/cve-check.bbclass > > > > > > > > +++ b/meta/classes/cve-check.bbclass > > > > > > > > @@ -78,6 +78,11 @@ CVE_CHECK_SKIP_RECIPE ?= "" > > > > > > > > # > > > > > > > > CVE_CHECK_IGNORE ?= "" > > > > > > > > > > > > > > > > +# Usually a CVE gets treated as patched when a patch > > > > > > > > with the > > > > > > > > name > > > > > > > > of the CVE > > > > > > > > +# gets applied. Basically this variable should not be > > > > > > > > used. > > > > > > > > But if > > > > > > > > there are > > > > > > > > +# other reasons to mark a CVE as patched it can be > > > > > > > > added to > > > > > > > > this > > > > > > > > list. > > > > > > > > +CVE_CHECK_PATCHED ?= "" > > > > > > > > > > > > We're not adding variables which are documented as > > > > > > "Basically > > > > > > this > > > > > > variable should not be used.". If you shouldn't need/use > > > > > > it, we > > > > > > don't > > > > > > need it. > > > > Ok, maybe I should change the description a little bit. Do you > > > > have > > > > some other preference? > > > > > > > > > > > > Can't you just use the ignore variable for the same end > > > > > > result? > > > > Nope. If I use a ignore list, the output in the SBOM will be > > > > set to > > > > "ignored", which is wrong, because it has been fixed. And > > > > that's > > > > the > > > > reason. > > > > > > > > I suspect "ignored" is a bad way to describe things. Ignore might > > mean > > the issue doesn't apply, has been fixed in some way or we really > > are > > ignoring it. What does the SBOM spec say about different field > > values? > > Should we be providing more reasoning than just adding to an ignore > > list? > > > > I'm a bit worried we're not solving the real problem here by adding > > a > > new variable we tell people not to use. The patch from Andrej tries to solves a real issue: The CVE checker distinguishes between two types of patches. Ignored (= not applicable) and patched. Patching is only supported by adding a real patch file to the SRC_URI. However, there are other ways a patch can be implemented. For example, a recipe that uses the git fetcher would update the git hash to a commit that contains a fix instead of applying a patch file to the recipe. But I fully agree that the comment (originally suggested by me when Andrej and I were discussing the solution) is bad. Maybe it should read as follows: Normally, a CVE is treated as patched when a patch with the name of the CVE is applied. CVE_CHECK_PATCHED allows to extend the list of patched CVEs without adding a patch file to SRC_URI. Regarding the SBOM: It is important for customers that the CVEs of a product with SBOM can be correctly identified as repaired or as ignored. However, I'm not sure if the SBOM part is properly addressed by the patch. The create-spdx.bbclass uses the function oe.cve_check.get_patched_cves(d) which should probably handle the new variable as well. We will check that and come up with a V2. Thank you and regards, Adrian > > > > Cheers, > > > > Richard > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- > > Links: You receive all messages sent to this group. > > View/Reply Online (#180915): > > https://lists.openembedded.org/g/openembedded-core/message/180915 > > Mute This Topic: https://lists.openembedded.org/mt/98703185/4454582 > > Group Owner: openembedded-core+owner@lists.openembedded.org > > Unsubscribe: > > https://lists.openembedded.org/g/openembedded-core/unsub > > [adrian.freihofer@gmail.com] > > -=-=-=-=-=-=-=-=-=-=-=- > >
Hi Andrej, On 05.05.23 at 13:18, Andrej Valek via lists.openembedded.org wrote: > CVE_CHECK_PATCHED - should contains an additional CVEs which have been > fixed and shouldn't be mark as vulnerable nor ignored. > > Signed-off-by: Andrej Valek <andrej.valek@siemens.com> > --- > meta/classes/cve-check.bbclass | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve-check.bbclass > index bd9e7e7445c..957ea0130dc 100644 > --- a/meta/classes/cve-check.bbclass > +++ b/meta/classes/cve-check.bbclass > @@ -78,6 +78,11 @@ CVE_CHECK_SKIP_RECIPE ?= "" > # > CVE_CHECK_IGNORE ?= "" > > +# Usually a CVE gets treated as patched when a patch with the name of the CVE > +# gets applied. Basically this variable should not be used. But if there are > +# other reasons to mark a CVE as patched it can be added to this list. > +CVE_CHECK_PATCHED ?= "" > + > # Layers to be excluded > CVE_CHECK_LAYER_EXCLUDELIST ??= "" > > @@ -284,6 +289,9 @@ def check_cves(d, patched_cves): > > cve_ignore = d.getVar("CVE_CHECK_IGNORE").split() > > + # add additional patched CVEs into existing patched list > + patched_cves.update(d.getVar("CVE_CHECK_PATCHED").split()) > + > import sqlite3 > db_file = d.expand("file:${CVE_CHECK_DB_FILE}?mode=ro") > conn = sqlite3.connect(db_file, uri=True) Thanks for the patch! However, we'd need you to add one thing to your git configuration, so that your patches are given an "Author" field which matches your "Signed-off-by" information. See https://www.openembedded.org/wiki/How_to_submit_a_patch_to_OpenEmbedded#Fixing_your_From_identity for details. Don't hesitate to send a patch test directly to me, if you wish. Thanks again, Michael.
On 8 May 2023, at 09:57, Adrian Freihofer via lists.openembedded.org <adrian.freihofer=gmail.com@lists.openembedded.org> wrote: > The patch from Andrej tries to solves a real issue: The CVE checker > distinguishes between two types of patches. Ignored (= not applicable) > and patched. Patching is only supported by adding a real patch file to > the SRC_URI. However, there are other ways a patch can be implemented. > For example, a recipe that uses the git fetcher would update the git > hash to a commit that contains a fix instead of applying a patch file > to the recipe. > > But I fully agree that the comment (originally suggested by me when > Andrej and I were discussing the solution) is bad. Maybe it should read > as follows: > > Normally, a CVE is treated as patched when a patch with the name of the > CVE is applied. CVE_CHECK_PATCHED allows to extend the list of patched > CVEs without adding a patch file to SRC_URI. > > Regarding the SBOM: It is important for customers that the CVEs of a > product with SBOM can be correctly identified as repaired or as > ignored. However, I'm not sure if the SBOM part is properly addressed > by the patch. The create-spdx.bbclass uses the function > oe.cve_check.get_patched_cves(d) which should probably handle the new > variable as well. We will check that and come up with a V2. So I’d suggest we deprecate CVE_CHECK_IGNORE and add a new, more flexible, variable instead. How about a CVE_STATUS, which doesn’t have a direct value but has flags for each CVE: # We moved to a git SHA that incorporates the fix CVE_STATUS[CVE-1234–0001] = “Patched” # We disabled frobnicate CVE_STATUS[CVE-1234-0002] = “Patched” # This is Windows-specific CVE_STATUS[CVE-1234-0003” = “Not Applicable” I’m not sure of the exact list of values the flags should accept beyond “patched” and “not applicable”. There probably does need to be a “reviewed and don’t consider this a problem” which feels like ‘ignored’ but I’m not a fan of that precise word. Is there any defined language that we can simply adopt? Cheers, Ross
On Tue, 2023-05-09 at 09:02 +0000, Ross Burton wrote: > On 8 May 2023, at 09:57, Adrian Freihofer via lists.openembedded.org <adrian.freihofer=gmail.com@lists.openembedded.org> wrote: > > The patch from Andrej tries to solves a real issue: The CVE checker > > distinguishes between two types of patches. Ignored (= not applicable) > > and patched. Patching is only supported by adding a real patch file to > > the SRC_URI. However, there are other ways a patch can be implemented. > > For example, a recipe that uses the git fetcher would update the git > > hash to a commit that contains a fix instead of applying a patch file > > to the recipe. > > > > But I fully agree that the comment (originally suggested by me when > > Andrej and I were discussing the solution) is bad. Maybe it should read > > as follows: > > > > Normally, a CVE is treated as patched when a patch with the name of the > > CVE is applied. CVE_CHECK_PATCHED allows to extend the list of patched > > CVEs without adding a patch file to SRC_URI. > > > > Regarding the SBOM: It is important for customers that the CVEs of a > > product with SBOM can be correctly identified as repaired or as > > ignored. However, I'm not sure if the SBOM part is properly addressed > > by the patch. The create-spdx.bbclass uses the function > > oe.cve_check.get_patched_cves(d) which should probably handle the new > > variable as well. We will check that and come up with a V2. > > So I’d suggest we deprecate CVE_CHECK_IGNORE and add a new, more > flexible, variable instead. > > How about a CVE_STATUS, which doesn’t have a direct value but has > flags for each CVE: > > # We moved to a git SHA that incorporates the fix > CVE_STATUS[CVE-1234–0001] = “Patched” > > # We disabled frobnicate > CVE_STATUS[CVE-1234-0002] = “Patched” > > # This is Windows-specific > CVE_STATUS[CVE-1234-0003” = “Not Applicable” > > I’m not sure of the exact list of values the flags should accept > beyond “patched” and “not applicable”. There probably does need to be > a “reviewed and don’t consider this a problem” which feels like > ‘ignored’ but I’m not a fan of that precise word. > > Is there any defined language that we can simply adopt? The question is probably what actions might someone want to take? We might want to separate out "N/A - configuration disabled" from "N/A - OS mismatch" and "CPE incorrect" for example? The reason being that someone would then know to look at things more closely if they were changing configuration, or building windows binaries? Given we already put fairly robust reasoning in comments already, should we capture that in a variable too? CVS_STATUS_REASONING[[CVE-1234-0003] = "issue only applies on windows" Cheers, Richard (thinking out loud)
Hi, On Tue, May 09, 2023 at 09:02:59AM +0000, Ross Burton wrote: > On 8 May 2023, at 09:57, Adrian Freihofer via lists.openembedded.org <adrian.freihofer=gmail.com@lists.openembedded.org> wrote: > > The patch from Andrej tries to solves a real issue: The CVE checker > > distinguishes between two types of patches. Ignored (= not applicable) > > and patched. Patching is only supported by adding a real patch file to > > the SRC_URI. However, there are other ways a patch can be implemented. > > For example, a recipe that uses the git fetcher would update the git > > hash to a commit that contains a fix instead of applying a patch file > > to the recipe. > > > > But I fully agree that the comment (originally suggested by me when > > Andrej and I were discussing the solution) is bad. Maybe it should read > > as follows: > > > > Normally, a CVE is treated as patched when a patch with the name of the > > CVE is applied. CVE_CHECK_PATCHED allows to extend the list of patched > > CVEs without adding a patch file to SRC_URI. > > > > Regarding the SBOM: It is important for customers that the CVEs of a > > product with SBOM can be correctly identified as repaired or as > > ignored. However, I'm not sure if the SBOM part is properly addressed > > by the patch. The create-spdx.bbclass uses the function > > oe.cve_check.get_patched_cves(d) which should probably handle the new > > variable as well. We will check that and come up with a V2. > > So I’d suggest we deprecate CVE_CHECK_IGNORE and add a new, more flexible, variable instead. Flexible but usefull and with clear definitions and checks which make sure that only those definitions are used. > How about a CVE_STATUS, which doesn’t have a direct value but has flags for each CVE: > > # We moved to a git SHA that incorporates the fix > CVE_STATUS[CVE-1234–0001] = “Patched” > > # We disabled frobnicate > CVE_STATUS[CVE-1234-0002] = “Patched” > > # This is Windows-specific > CVE_STATUS[CVE-1234-0003” = “Not Applicable” > > I’m not sure of the exact list of values the flags should accept beyond “patched” and “not applicable”. There probably does need to be a “reviewed and don’t consider this a problem” which feels like ‘ignored’ but I’m not a fan of that precise word. Sounds ok as long as the output reports as easy to read as now. > Is there any defined language that we can simply adopt? Since a lot of people talk about SPDX solving these issues would be nice to know how that is going to work. I can't parse https://spdx.github.io/spdx-spec/v2.3/how-to-use/#k17-linking-to-a-code-fix-for-a-security-issue and figure out how to mark a CVE issue which has been ignored after analysis. Debian has a bit more complex state for each CVE (and also non-CVE security issues) which relates to package and distro versions. I did not find clear definition of the states but at least https://security-tracker.debian.org/tracker/data/json has the raw data available. Ubuntu seems to follow Debian a bit but then also adds more complex states in the (at least) public database at https://ubuntu.com/security/cves?q=CVE-2023-26117&package=&priority=&version=&status= I think the data coming from CVE checker needs to serve the needs of the distro maintainers so that their life is easier. SPDX and SBOM are supposed to help but I'm afraid that they don't unless they actually help with the maintenance and start to solve the problems there. I'm used to the CVE checker ignore list (previously whitelist) and know how to use it. Wether the data comes per CVE or as lists for each of the state as variables is a small detail, as long as the generated report is readable. Cheers, -Mikko
On 9/05/23 9:32 pm, Mikko Rapeli wrote: > On Tue, May 09, 2023 at 09:02:59AM +0000, Ross Burton wrote: >> On 8 May 2023, at 09:57, Adrian Freihofer via lists.openembedded.org<adrian.freihofer=gmail.com@lists.openembedded.org> wrote: >> Is there any defined language that we can simply adopt? > Since a lot of people talk about SPDX solving these issues would be nice > to know how that is going to work. I can't parse > https://spdx.github.io/spdx-spec/v2.3/how-to-use/#k17-linking-to-a-code-fix-for-a-security-issue > and figure out how to mark a CVE issue which has been ignored after > analysis. Perhaps this? https://spdx.github.io/spdx-spec/v2.3/how-to-use/#k16-linking-to-a-vulnerability-disclosure-document To communicate that a package is not vulnerable to a specific vulnerability it is recommended to reference a web page indicating why given vulnerabilities are not applicable. |"externalRefs" : [ { "referenceCategory" : "SECURITY", "referenceLocator" : "https://example.com/product-x/security-info.html", "referenceType" : "advisory" } ] |
Hi, On Wed, May 10, 2023 at 09:37:13AM +1200, Douglas Royds wrote: > On 9/05/23 9:32 pm, Mikko Rapeli wrote: > > On Tue, May 09, 2023 at 09:02:59AM +0000, Ross Burton wrote: > > > On 8 May 2023, at 09:57, Adrian Freihofer via lists.openembedded.org<adrian.freihofer=gmail.com@lists.openembedded.org> wrote: > > > Is there any defined language that we can simply adopt? > > Since a lot of people talk about SPDX solving these issues would be nice > > to know how that is going to work. I can't parse > > https://spdx.github.io/spdx-spec/v2.3/how-to-use/#k17-linking-to-a-code-fix-for-a-security-issue > > and figure out how to mark a CVE issue which has been ignored after > > analysis. > > > Perhaps this? > > https://spdx.github.io/spdx-spec/v2.3/how-to-use/#k16-linking-to-a-vulnerability-disclosure-document > > To communicate that a package is not vulnerable to a specific > vulnerability it is recommended to reference a web page indicating > why given vulnerabilities are not applicable. > > |"externalRefs" : [ { "referenceCategory" : "SECURITY", > "referenceLocator" : > "https://example.com/product-x/security-info.html", "referenceType" > : "advisory" } ] | Thanks but IMO this does not encode the information that analysis has been done and the issue can safely be ignored, but I'm not an SPDX expert, and frankly I should not need to be. In recipes CVE_CHECK_IGNORE variable the ignore list is clear, obvious, and there is usually a comment or a commit message explaining why. And the reports generated by cve-check.bbclass for recipes and images show that the CVE issue can be ignored and maintainer should check the CVEs with "Unpatched" status instead. Would be nice for these tools to firstly support yocto upstream stable and LTS maintainers work in detecting and fixing CVE issues, and secondly support maintaining CVE security issue/patching status of older releases with complex layer configurations, when anyone has to use an old release due to BSP etc dependencies (fact of life which IMO should not be completely ignored). I have backported the cve-check.bbclass and other CVE management related patches to really old yocto releases and these frankly saved the product from being the usual embedded SW security nightmare to actually have only a few known minor known CVE patching issues when shipping to customers. Older versions of SPDX standard and open source license checks helped to identify embedded open source SW but did not really help in the yocto operating system/rootfs side CVE security patching. Cheers, -Mikko
diff --git a/meta/classes/cve-check.bbclass b/meta/classes/cve-check.bbclass index bd9e7e7445c..957ea0130dc 100644 --- a/meta/classes/cve-check.bbclass +++ b/meta/classes/cve-check.bbclass @@ -78,6 +78,11 @@ CVE_CHECK_SKIP_RECIPE ?= "" # CVE_CHECK_IGNORE ?= "" +# Usually a CVE gets treated as patched when a patch with the name of the CVE +# gets applied. Basically this variable should not be used. But if there are +# other reasons to mark a CVE as patched it can be added to this list. +CVE_CHECK_PATCHED ?= "" + # Layers to be excluded CVE_CHECK_LAYER_EXCLUDELIST ??= "" @@ -284,6 +289,9 @@ def check_cves(d, patched_cves): cve_ignore = d.getVar("CVE_CHECK_IGNORE").split() + # add additional patched CVEs into existing patched list + patched_cves.update(d.getVar("CVE_CHECK_PATCHED").split()) + import sqlite3 db_file = d.expand("file:${CVE_CHECK_DB_FILE}?mode=ro") conn = sqlite3.connect(db_file, uri=True)
CVE_CHECK_PATCHED - should contains an additional CVEs which have been fixed and shouldn't be mark as vulnerable nor ignored. Signed-off-by: Andrej Valek <andrej.valek@siemens.com> --- meta/classes/cve-check.bbclass | 8 ++++++++ 1 file changed, 8 insertions(+)