Skip to content
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

- add conan.lock to the list #59

Merged
merged 30 commits into from
Jan 19, 2023
Merged

- add conan.lock to the list #59

merged 30 commits into from
Jan 19, 2023

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Dec 15, 2022

/cc @prince-chrismc

there are several formats of conan lockfile available in the wild (lockfiles are versioned separately from the conan client itself, as not every conan release changes the format of lockfile):

  • initial, unversioned (0.0) - added in conan 1.17
  • 0.1 added in conan 1.18, added versioning to the lockfile format
  • 0.2 added in conan 1.21, added deterministic lockfiles
  • 0.3 added in conan 1.22, changed how nodes reference each other
  • 0.4 added in conan 1.28, the format mostly used currently in production and encountered most often
  • 0.5 is used by conan 2.0+ (currently, as of December 2022, still in beta state, and not yet released)

formats 0.0 - 0.4 are mostly similar and use graph_lock with a list of nodes.
format 0.5 is a major overhaul, it's a completely new implementation developed from scratch.

some information about 0.0 - 0.4 lockfiles (current format used in production):

more information about 0.5 lockfiles used in conan 2.0:

the format itself is not documented, but can be learnt by studying the source code graph_lock.py. as it's currently not expected lockfiles to be manually or automatically edited by users or tools, they are only generated by the conan client itself during the graph resolution process, and afterwards, lockfiles are fully immutable.

for the version comparison, conan client itself uses version ranges to compare versions, but it's only for packages that follow SemVer (a majority of C and C++ libraries don't follow SemVer, or even predate it). as different libraries use various versioning schemes, conan client cannot make any assumtions.
however, lockfile represents a resolved graph state, with all the version ranges already computed. therefore, versions in lockfiles are compared as regular strings.

@prince-chrismc
Copy link

It needs to be implemented in https://github.com/google/osv-scanner/tree/main/pkg/lockfile 🙊

Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4
Copy link
Contributor Author

SSE4 commented Dec 18, 2022

@prince-chrismc I've added some parser

@G-Rath G-Rath self-requested a review December 18, 2022 18:04
PythonRequires []string `json:"python_requires,omitempty"`
}

const ConanEcosystem Ecosystem = "conan.io"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just flagging this isn't a valid ecosystem currently per the OSV spec - I'll leave it to @another-rex & @oliverchang to decide what to do about this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!

We should at the very least add a TODO here to say that this is tentative and subject to change depending on the OSV schema.

This is definitely a useful addition to have for the OSV-Scanner, but the database itself also currently does not have conan.io advisories. Is there also an existing Conan vuln DB that we could have publishing OSV entries?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there also an existing Conan vuln DB that we could have publishing OSV entries?

Conan is the C++ package manager 😉 We have ConanCenter which offers a ton of C and C++ packages. https://cve.mitre.org/index.html or https://nvd.nist.gov/ are the types of places I'd expect find know vulnerabilities.

https://conan.io/center/openssl has numerous ones https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=openssl.
openssl/3.0.5 has CVE-2022-3602 for example.

I saw in the readme these are listed as alias could they not be the IDs as well? Some like the Openssl Alpine that I see on https://osv.dev/list?ecosystem=Alpine&q=openssl was my initial reaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I've opened a PR to ovs-schema: ossf/osv-schema#101
for the database, sorry, but I have no idea. maybe @prince-chrismc or @jcar87 know and can comment. perhaps, X-Ray might have some sort of database, but I am not familiar with it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a public database 😉 But public database that already exists are welcomed!


func parseConanV2Lock(lockfile ConanLockFile) []PackageDetails {

packages := make([]PackageDetails, 0, len(lockfile.Requires)+len(lockfile.BuildRequires)+len(lockfile.PythonRequires))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to wrap these in uint64 to make CodeQL happy, otherwise it will flag this as a potential vulnerability since the return type of len technically allows negative numbers even though in practice that should never be possible.

See here for an example of how I've addressed that in the past

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I am not that familiar with Go or GoLang, so negative length or size type sounds a bit suspicious for me. fixed it.

@G-Rath
Copy link
Collaborator

G-Rath commented Dec 18, 2022

@SSE4 I've done an initial review and nothing huge has jumped out at me as needing to be changed - I'm not familiar with this ecosystem/tool at all; would you happen to have any references to the lockfile (ideally a spec would be great, but not all ecosystems have those).

It would also be good to know how the version comparison is actually done if you know that too.

Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4
Copy link
Contributor Author

SSE4 commented Dec 19, 2022

@SSE4 I've done an initial review and nothing huge has jumped out at me as needing to be changed - I'm not familiar with this ecosystem/tool at all; would you happen to have any references to the lockfile (ideally a spec would be great, but not all ecosystems have those).

It would also be good to know how the version comparison is actually done if you know that too.

thanks, I've updated the PR description. lemme know if you have any further question.

@SSE4
Copy link
Contributor Author

SSE4 commented Jan 4, 2023

@G-Rath how do we move this forward? please let me know if you need anything else from my side.

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 4, 2023

@SSE4 the main thing that needs addressing is this comment - otherwise everything looks ok

pkg/lockfile/parse-conan-lock.go Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor Author

SSE4 commented Jan 5, 2023

@SSE4 the main thing that needs addressing is this comment - otherwise everything looks ok

okay, I've changed ecosystem name conan.io to conan and added TODO you have suggested.

pkg/lockfile/parse-conan-lock.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got a couple of nitty comments about newlines

Comment on lines 102 to 103
for _, node := range lockfile.GraphLock.Nodes {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
for _, node := range lockfile.GraphLock.Nodes {
for _, node := range lockfile.GraphLock.Nodes {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a change here 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think GitHub bugged out - I've remade the change here across more lines so hopefully it'll stick

pkg/lockfile/parse-conan-lock.go Outdated Show resolved Hide resolved
pkg/lockfile/parse-conan-lock.go Outdated Show resolved Hide resolved
pkg/lockfile/parse-conan-lock.go Outdated Show resolved Hide resolved
Comment on lines 88 to 94
parts = strings.SplitN(ref, "/", 2)
if len(parts) == 2 {
reference.Name = parts[0]
reference.Version = parts[1]
} else {
reference.Version = ref
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect I'm about to open a can of worms with this (which I missed in my first review, sorry about that), but this looks to me like it's possible for a package to not have a name - is that true? technically the osv.dev API allows that if you have a commit, but it's not really expected elsewhere.

it also looks like none of your tests are actually covering this line (unless I've missed something)

Copy link
Contributor Author

@SSE4 SSE4 Jan 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it's possible from the code perspective: https://github.com/conan-io/conan/blob/478395863b7b0ac0ec400c91410f43ba66754e75/conans/model/ref.py#L54
however I don't remember when exactly does it happen
@prince-chrismc do you remember if it's a legit situation? if not, we probably can just raise for invalid reference.
(in practice I think I've encountered some CLI errors trying to make a package without name 🤔 - e.g. ERROR: conanfile didn't specify name)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the official packages that are open-source that could possibly be in public databases with CVE will have a name.

It's technically possible for someone to have private packages but I would safe those are safe to skip.

Perhaps a warning or just skipping the package is an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just got some responses:

It is possible to have self.version defined for a consumer, but not have self.name defined
In conan export or conan create it should never happen, I understand that it has reached the lockfile because it is a consumer
Well, not having to define a name, a conanfile.txt has neither a name nor a version, a conanfile.py also works fine without a name or version
But if you want to give it a version, you can.

so, I think they might theoretically appear in a lockfile, but they should be completely safe to skip.
I'll try to add some test case, if I can generate such a lockfile, otherwise, I'll just skip them in code (if Name is missing or empty).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added code to skip packages with no name

SSE4 and others added 4 commits January 9, 2023 09:03
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
@SSE4 SSE4 requested a review from G-Rath January 12, 2023 13:11
Copy link
Collaborator

@another-rex another-rex left a 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, just waiting for (ossf/osv-schema#101) to be accepted before merging this in.

pkg/lockfile/parse_test.go Outdated Show resolved Hide resolved
pkg/lockfile/parse.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor Author

SSE4 commented Jan 18, 2023

Looks good to me, just waiting for (ossf/osv-schema#101) to be accepted before merging this in.

it's merged now

@SSE4 SSE4 requested review from prince-chrismc and oliverchang and removed request for oliverchang and prince-chrismc January 19, 2023 03:24
@oliverchang oliverchang requested review from another-rex and removed request for prince-chrismc January 19, 2023 04:41
@another-rex another-rex merged commit 2611f9f into google:main Jan 19, 2023
@SSE4 SSE4 deleted the patch-1 branch January 20, 2023 05:03
hayleycd pushed a commit that referenced this pull request Mar 9, 2023
* - add conan.lock to the list

* - conan lockfile parser

Signed-off-by: SSE4 <tomskside@gmail.com>

* - make CodeQL happy, use explicit uint64 cast

Signed-off-by: SSE4 <tomskside@gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

* Update pkg/lockfile/parse-conan-lock.go

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* - skip references with no name

Signed-off-by: SSE4 <tomskside@gmail.com>

* - add test for packages with no name specified

Signed-off-by: SSE4 <tomskside@gmail.com>

* Update README.md

* Update parse_test.go

* Update parse.go

* - fix test

Signed-off-by: SSE4 <tomskside@gmail.com>

Signed-off-by: SSE4 <tomskside@gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
julieqiu pushed a commit to julieqiu/osv-scanner that referenced this pull request May 2, 2023
* - add conan.lock to the list

* - conan lockfile parser

Signed-off-by: SSE4 <tomskside@gmail.com>

* - make CodeQL happy, use explicit uint64 cast

Signed-off-by: SSE4 <tomskside@gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

* Update pkg/lockfile/parse-conan-lock.go

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* - skip references with no name

Signed-off-by: SSE4 <tomskside@gmail.com>

* - add test for packages with no name specified

Signed-off-by: SSE4 <tomskside@gmail.com>

* Update README.md

* Update parse_test.go

* Update parse.go

* - fix test

Signed-off-by: SSE4 <tomskside@gmail.com>

Signed-off-by: SSE4 <tomskside@gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
julieqiu pushed a commit to julieqiu/osv-scanner that referenced this pull request May 2, 2023
* - add conan.lock to the list

* - conan lockfile parser

Signed-off-by: SSE4 <tomskside@gmail.com>

* - make CodeQL happy, use explicit uint64 cast

Signed-off-by: SSE4 <tomskside@gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

* Update pkg/lockfile/parse-conan-lock.go

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* Update pkg/lockfile/parse-conan-lock.go

Co-authored-by: Gareth Jones <Jones258@Gmail.com>

* - skip references with no name

Signed-off-by: SSE4 <tomskside@gmail.com>

* - add test for packages with no name specified

Signed-off-by: SSE4 <tomskside@gmail.com>

* Update README.md

* Update parse_test.go

* Update parse.go

* - fix test

Signed-off-by: SSE4 <tomskside@gmail.com>

Signed-off-by: SSE4 <tomskside@gmail.com>
Co-authored-by: Gareth Jones <Jones258@Gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants