-
Notifications
You must be signed in to change notification settings - Fork 40
Malicious code scanner #614
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: dev
Are you sure you want to change the base?
Conversation
b35743f to
e7bdd1c
Compare
e7bdd1c to
4ed08e3
Compare
4ed08e3 to
af08b28
Compare
af08b28 to
9da5f8c
Compare
1fe6809 to
16aab3c
Compare
16aab3c to
e846bff
Compare
e846bff to
ff68bae
Compare
attiasas
left a 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.
Nice work, Check out my comments:
- Update branch, merge dev and fix conflicts
- Make sure tests are passing after updating the AM version and running them
- Don't forget to merge dependent PR and update replaces before merging.
- Update PR description
| // Expected number of Secrets issues | ||
| Secrets int | ||
| // Expected number of Malicious Code issues | ||
| MaliciousCode int |
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.
you added the expected malicious code issues but you are not using it.
you need to add code to ValidateScanTypeCount as well
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 do use it, why do I have to add to ValidateScanTypeCount?
| if cmd.customAnalyzerManagerPath != "" { | ||
| scanner.AnalyzerManager.AnalyzerManagerFullPath = cmd.customAnalyzerManagerPath | ||
| } else { | ||
| if scanner.AnalyzerManager.AnalyzerManagerFullPath, err = jas.GetAnalyzerManagerExecutable(); err != nil { | ||
| return fmt.Errorf("failed to set analyzer manager executable path: %w", err) | ||
| } | ||
| } |
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 custom path is not empty (provided) we don't need to download the AM to the default dir...
similar to audit:
if cmd.customAnalyzerManagerPath == "" {
if generalError = jas.DownloadAnalyzerManagerIfNeeded(threadId); generalError != nil {
return fmt.Errorf("failed to download analyzer manager: %s", generalError.Error())
}
if scanner.AnalyzerManager.AnalyzerManagerFullPath, generalError = jas.GetAnalyzerManagerExecutable(); generalError != nil {
return fmt.Errorf("failed to set analyzer manager executable path: %s", generalError.Error())
}
} else {
scanner.AnalyzerManager.AnalyzerManagerFullPath = cmd.customAnalyzerManagerPath
log.Debug(clientutils.GetLogMsgPrefix(threadId, false) + "using custom analyzer manager binary 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.
ok
| if len(workingDirs) == 0 { | ||
| currentDir, err := coreutils.GetWorkingDirectory() | ||
| if err != nil { | ||
| cmdResults.AddGeneralError(fmt.Errorf("failed to get current working directory: %w", err), false) | ||
| return | ||
| } | ||
| cmdResults.NewScanResults(results.ScanTarget{Target: currentDir, Name: filepath.Base(currentDir)}) |
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.
not possiable, already has at least one value after coreutils.GetFullPathsWorkingDirs(cmd.workingDirs)
| if len(workingDirs) == 0 { | |
| currentDir, err := coreutils.GetWorkingDirectory() | |
| if err != nil { | |
| cmdResults.AddGeneralError(fmt.Errorf("failed to get current working directory: %w", err), false) | |
| return | |
| } | |
| cmdResults.NewScanResults(results.ScanTarget{Target: currentDir, Name: filepath.Base(currentDir)}) |
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.
right
| } | ||
|
|
||
| if len(cmdResults.Targets) == 0 { | ||
| log.Warn("No scan targets were detected. Proceeding with empty scan...") |
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.
| log.Warn("No scan targets were detected. Proceeding with empty scan...") | |
| log.Warn("No scan targets were detected.") |
you are returning 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.
Right
| func logScanPaths(workingDirs []string) { | ||
| if len(workingDirs) == 0 { | ||
| return | ||
| } | ||
| if len(workingDirs) == 1 { | ||
| log.Info("Scanning path:", workingDirs[0]) | ||
| return | ||
| } | ||
| log.Info("Scanning paths:", strings.Join(workingDirs, ", ")) | ||
| } |
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.
| func logScanPaths(workingDirs []string) { | |
| if len(workingDirs) == 0 { | |
| return | |
| } | |
| if len(workingDirs) == 1 { | |
| log.Info("Scanning path:", workingDirs[0]) | |
| return | |
| } | |
| log.Info("Scanning paths:", strings.Join(workingDirs, ", ")) | |
| } | |
| func logScanPaths(workingDirs []string) { | |
| if len(workingDirs) == 0 { | |
| return | |
| } | |
| if len(workingDirs) == 1 { | |
| log.Debug("Scanning path:", workingDirs[0]) | |
| return | |
| } | |
| log.Debug("Scanning paths:", strings.Join(workingDirs, ", ")) | |
| } |
I would move it to DEBUG since we already outputing targets later in Info
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.
ok
ff68bae to
7886db7
Compare
attiasas
left a 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.
Check out my comments.
Also, make sure test are passing after updating the AM version with the scanner code
| return jas.CreateScannersConfigFile(mal.configFileName, configFileContent, jasutils.MaliciousCode) | ||
| } | ||
|
|
||
| func (mal *MaliciousScanManager) getExcludePatterns(exclusions ...string) []string { |
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.
you have: jas.GetExcludePatterns use it insead or adjust it if needed
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 can I use it without jfrogapps module?
cli/docs/flags.go
Outdated
| Url, XrayUrl, user, password, accessToken, ServerId, Threads, InsecureTls, | ||
| }, | ||
| MaliciousScan: { | ||
| Url, XrayUrl, user, password, accessToken, ServerId, Threads, InsecureTls, OutputFormat, MinSeverity, AnalyzerManagerCustomPath, WorkingDirs, scanProjectKey, |
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 should add a similar flag with different description if command is not hidden
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.
ok
devbranch.go vet ./....go fmt ./....Description:
New malicious scanners which check for malicious code on the source code. For example, models such as pickle may contain malicious code in their code, so now jf malicious-scan can scan this pickles and show in a table what is the malicious code.
Example: