New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: long version of go dependency unable to inserted into software table (CVE-2020-36569) #5221
fix: long version of go dependency unable to inserted into software table (CVE-2020-36569) #5221
Conversation
commit message should start with a lowercase fix otherwise the pipeline breaks @srini00088 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there, I believe this would be really help for one of the current issues i.e. issue with 'CVE-2020-36569' without harming any other.
a875ecd
to
e5a2f29
Compare
@gregoranders thanks, have amended the commit message |
@srini00088 please add space and commit is with small letter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srini00088 kudos to you great job. Proactively solves the issue. I believe this will solve the issue
Any tentative timelines on when this fix can get released? It's critical. |
AFAIU this ain't complete yet. You need to modify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
How about a longer field than 75. Lets crank it up to at least 100 to buy some time. |
…ze scripts and properties
@@ -0,0 +1,6 @@ | |||
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest a higher number than 75, because it is the exact the size for this specific entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about 255 like other COLUMNs or even more. Is their a concret reason for 75 except that the current CVE needs 75?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am all for 255
, at least I don't see a reason to not make it reasonably bigger, otherwise 75
is just us waiting for the next one to happen.
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); | ||
ALTER TABLE software ALTER COLUMN versionEndIncluding SET DATA TYPE VARCHAR(75); | ||
ALTER TABLE software ALTER COLUMN versionStartExcluding SET DATA TYPE VARCHAR(75); | ||
ALTER TABLE software ALTER COLUMN versionStartIncluding SET DATA TYPE VARCHAR(75); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); | |
ALTER TABLE software ALTER COLUMN versionEndIncluding SET DATA TYPE VARCHAR(75); | |
ALTER TABLE software ALTER COLUMN versionStartExcluding SET DATA TYPE VARCHAR(75); | |
ALTER TABLE software ALTER COLUMN versionStartIncluding SET DATA TYPE VARCHAR(75); | |
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(100); | |
ALTER TABLE software ALTER COLUMN versionEndIncluding SET DATA TYPE VARCHAR(100); | |
ALTER TABLE software ALTER COLUMN versionStartExcluding SET DATA TYPE VARCHAR(100); | |
ALTER TABLE software ALTER COLUMN versionStartIncluding SET DATA TYPE VARCHAR(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be safer to trim any data read from CVE to not risk database length violations? Otherwise any faulty data in CVE entries will make the plugin fail again. |
@@ -0,0 +1,6 @@ | |||
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as @Keymaster65 already suggested, i would also prefer to take the column length of 255 chars:
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); | |
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(255); | |
ALTER TABLE software ALTER COLUMN versionEndIncluding SET DATA TYPE VARCHAR(255); | |
ALTER TABLE software ALTER COLUMN versionStartExcluding SET DATA TYPE VARCHAR(255); | |
ALTER TABLE software ALTER COLUMN versionStartIncluding SET DATA TYPE VARCHAR(255); |
@@ -0,0 +1,6 @@ | |||
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am all for 255
, at least I don't see a reason to not make it reasonably bigger, otherwise 75
is just us waiting for the next one to happen.
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); | ||
ALTER TABLE software ALTER COLUMN versionEndIncluding SET DATA TYPE VARCHAR(75); | ||
ALTER TABLE software ALTER COLUMN versionStartExcluding SET DATA TYPE VARCHAR(75); | ||
ALTER TABLE software ALTER COLUMN versionStartIncluding SET DATA TYPE VARCHAR(75); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest that this be merged as a hotfix asap so that it works for now and be optimized and discussed afterwards. |
I will update this PR and release an update today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder that you should also update the type in the create table software
in the initialize[_xxx].sql
CREATE TABLE software (cveid INT, cpeEntryId INT, versionEndExcluding VARCHAR(60), versionEndIncluding VARCHAR(60), |
I don't know the project, but when these columns have been added to the table with the script upgrade_5.1.sql
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(60); |
@@ -0,0 +1,6 @@ | |||
ALTER TABLE software ALTER COLUMN versionEndExcluding SET DATA TYPE VARCHAR(75); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not this file be named upgrade_5.2.1.sql
?
@@ -21,7 +21,7 @@ data.file_name=odc.mv.db | |||
### if you increment the DB version then you must increment the database file path | |||
### in the mojo.properties, task.properties (maven and ant respectively), and | |||
### the gradle PurgeDataExtension. | |||
data.version=5.2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is used to choose the update file (org.owasp.dependencycheck.data.nvdcve.DatabaseManager#DB_STRUCTURE_UPDATE_RESOURCE
) when h2
is used (org.owasp.dependencycheck.data.nvdcve.DatabaseManager#updateSchema
).
Analysis crashes when the upgrade file does not exist. Along with my previous comment I would say that this property should be equal to 5.2.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not this property, but the database version property in your H2 database that governs which upgrade SQL is searched for. This property governs the expected database version (and if actual is lower than that will search for upgrade scripts to update the H2 database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
File must be named according to the database-version it upgrades.
@jeremylong did a fix-up of the update-script name, before spotting your response. Will leave the further polishing to you. |
Does utils\src\test\resources\dependencycheck.properties also need to be updated? |
see #5229 |
Could you please upgrade gradle plugin to 7.4.4 as well? Latest is still 7.4.3 https://plugins.gradle.org/plugin/org.owasp.dependencycheck |
Looks like it's showing 7.4.4 now. |
@jeremylong could you update the jenkins plugin too please? |
@jeremylong same here, please update this to jenkins please |
Ah, I missed that. Maybe the plugin didn't need updating. Excellent. |
Fixes Issue
Fix: long version of go dependency unable to inserted into software table (CVE-2020-36569)
Description of Change
Alter's column size to varchar 75
Please add a description of the proposed change
Alters to a minimum possible value of version columns to 75 characters. Could be more or less as well.
Have test cases been added to cover the new functionality?
no