-
Notifications
You must be signed in to change notification settings - Fork 81
CVE-2025-8088 WinRAR Detector #1135 #1187
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
base: main
Are you sure you want to change the base?
Conversation
@erikvarga |
@@ -67,6 +68,8 @@ var Untested = InitMap{ | |||
cve202011978.Name: {cve202011978.New}, | |||
// CVE-2024-2912 BentoML detector. | |||
cve20242912.Name: {cve20242912.New}, | |||
// CVE-2025-8088 WinRAR RCE detector |
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.
Can you add a new CVE list (with just this one detector) instead of adding this to Untested? Also add it to All and detectorNames.
Vulnerability: osvschema.Vulnerability{ | ||
ID: "CVE-2025-8088", | ||
Summary: "WinRAR path traversal vulnerability (<7.13)", | ||
Details: "Detects WinRAR installs or portables < 7.13, using robust and reliable PE analysis.", |
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.
The finding details should be about the vulnerability, i.e. "A path traversal vulnerability affecting the Windows version of WinRAR allows the attackers to execute arbitrary code by..."
}}} | ||
} | ||
|
||
// Scan checks for the presence of the BentoML CVE-2025-8088 vulnerability on the filesystem. |
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.
BentoML? You mean WinRAR?
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.
My bad, it has been mistakenly copied when creating stub. Kindly ignore, will fix and create PR
|
||
vuln := &inventory.PackageVuln{ | ||
Package: pkg, | ||
Vulnerability: osvschema.Vulnerability{ |
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.
Return the same Vulnerability as in DetectedFinding() here. For other extractors we usually have a shared helper lib, e.g.
func (d Detector) DetectedFinding() inventory.Finding { |
Vulnerability: osvschema.Vulnerability{ | ||
ID: "CVE-2025-8088", | ||
Summary: "WinRAR vulnerable version detected (installed package)", | ||
Details: fmt.Sprintf("Installed WinRAR %s detected via package index", normalizedVersion), |
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.
Details should be the same for each instance of the vulnerability. If you want to store instance-specific info, add an "extra" key to the DatabaseSpecific field. See as an example:
"extra": fmt.Sprintf("%s %s %s", pkg.Name, pkg.Version, strings.Join(pkg.Locations, ", ")), |
|
||
if isAffectedVersion(normalizedVersion) { | ||
log.Infof("Vulnerable WinRAR package found(Delete them): %s, Package: %s, Version: %s", path, prodName, normalizedVersion) | ||
pkg := &extractor.Package{ |
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.
Since this isn't a package that was found during the extraction step it's fine to leave this out from the vuln.
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.
But the portables and non-default installations will be found in this step only. May I know why we need to leave this packages even if it is vulnerable?
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.
The main concern is that this is a package outside of Inventory.Packages which breaks assumptions further down the code like the serialization of PackageVulns into a proto (where we'll only store a reference to the field in Inventory.Packages - this is still WIP and hasn't been submitted though.)
If we remove the custom file walking logic though then this shouldn't be a problem in either case.
Metadata: nil, | ||
} | ||
|
||
affected := []osvschema.Affected{{ |
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.
Same as for step 1, pls move the vuln creation logic into a helper func and use it both here and in DetectedFinding (with potentially different databaseSpecific data)
|
||
// Indicator 2: PE resource analysis - extract version info from PE resources | ||
// Now check PE resources for ALL .exe/.dll files, not just regex matches | ||
version, prodName := extractPEVersion(path) |
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.
We already extracted this information in the dotnetpe extractor AFAIK.
How about you have a separate step that looks at already extracted dotnetpe Packages and does name / version matching on those? And then you only need to do the file name matching here.
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.
If we could reuse this, extracting PE info for portables and non-default location installations will get covered. So, this covers the concern, right?
#1187 (comment)
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.
The PE extractor checks for PE executables in any location, not just default installs. So its results should already contain any info extracted here.
prodName = "RAR" | ||
} | ||
|
||
archRegex := regexp.MustCompile(`(?i)(x64|x86|arm64|arm32|portable)`) |
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.
Make regexps global vars so they're compiled at init time.
|
||
// isAffectedVersion returns true if version < 7.13 | ||
func isAffectedVersion(ver string) bool { | ||
parts := strings.Split(ver, ".") |
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.
We have a built-in version comparison library in SCALIBR:
Line 43 in 396e8cf
func Parse(str string, ecosystem string) (Version, error) { |
You can do something like semantic.Parse(ver).CompareStr("7.13")
Hi @erikvarga , I wanted to kindly follow up on my earlier comments regarding the issue comments for the PR. |
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.
Sorry for the delay, team was low on capacity so couldn't find time to give another review round until now
} | ||
} | ||
|
||
// === Phase 2: Filesystem scan (existing logic) === |
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.
dotnetpe is supposed to find executables from any directory within the ScanRoot - it just checks if the binary has a correct extension:
if !slices.Contains(peExtensions, ext) { |
In your SCALIBR CLI invocation you set the root to C:\Users\ganes\Desktop\public-sharables
so it should find the binaries from under there I believe. Not sure how it found anything under C:\Program Files
if that wasn't the path specified. Can you try with --root= instead of -root?
I think it's fine to leave out the deepScan parameter - supposedly all the potential executables should have been found by the inventory extractors already and we shouldn't need to re-walk the filesystem. So it should be fine to only implement the deepScan=false method which goes through the PackageIndex but doesn't traverse the filesystem.
|
||
// Indicator 2: PE resource analysis - extract version info from PE resources | ||
// Now check PE resources for ALL .exe/.dll files, not just regex matches | ||
version, prodName := extractPEVersion(path) |
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.
The PE extractor checks for PE executables in any location, not just default installs. So its results should already contain any info extracted here.
|
||
if isAffectedVersion(normalizedVersion) { | ||
log.Infof("Vulnerable WinRAR package found(Delete them): %s, Package: %s, Version: %s", path, prodName, normalizedVersion) | ||
pkg := &extractor.Package{ |
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.
The main concern is that this is a package outside of Inventory.Packages which breaks assumptions further down the code like the serialization of PackageVulns into a proto (where we'll only store a reference to the field in Inventory.Packages - this is still WIP and hasn't been submitted though.)
If we remove the custom file walking logic though then this shouldn't be a problem in either case.
This PR adds a detector for CVE-2025-8088 (WinRAR RCE vulnerability) in osv-scalibr.
The detector supports:
This ensures broader coverage by detecting vulnerable WinRAR executables even outside standard installation directories.