Skip to content

Suppress credscan false positives#38026

Merged
aik-jahoda merged 11 commits intodotnet:masterfrom
aik-jahoda:jajahoda/credscan1
Jul 23, 2020
Merged

Suppress credscan false positives#38026
aik-jahoda merged 11 commits intodotnet:masterfrom
aik-jahoda:jajahoda/credscan1

Conversation

@aik-jahoda
Copy link
Contributor

@aik-jahoda aik-jahoda commented Jun 17, 2020 •
edited
Loading

In libraries, there is a lot of credscan false positives. We use a lot of plaintext passwords and certificates in networking test code. All the passwords and certificates are generated only for test purpose and they are considered safe.

To prevent false positives next time this PR introduces a helper class to explicitly mark the test passwords and certificates.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Dotnet-GitSync-Bot commented Jun 17, 2020

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

@aik-jahoda aik-jahoda marked this pull request as ready for review June 18, 2020 13:32
aik-jahoda
aik-jahoda commented Jun 18, 2020
View reviewed changes
eng/common/internal-feed-operations.ps1 Outdated
}

if (($endpoints | Measure-Object).Count -gt 0) {
# [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="Endpoint code example")]
Copy link
Contributor Author

@aik-jahoda aik-jahoda Jun 18, 2020

Choose a reason for hiding this comment

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

yeah, this line suppress false positive in the comment below

@aik-jahoda aik-jahoda marked this pull request as draft June 18, 2020 14:12
aik-jahoda
aik-jahoda commented Jul 2, 2020
View reviewed changes
aik-jahoda
aik-jahoda commented Jul 9, 2020
View reviewed changes
@jaredpar jaredpar mentioned this pull request Jul 9, 2020
Closed
ManickaP
ManickaP reviewed Jul 10, 2020
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

I'd mostly go with the comment suppression instead of obfuscation.

@aik-jahoda aik-jahoda force-pushed the jajahoda/credscan1 branch from 3e3ef19 to 5db9bfa Compare July 13, 2020 13:15
@aik-jahoda aik-jahoda marked this pull request as ready for review July 13, 2020 13:34
@aik-jahoda aik-jahoda requested a review from a team July 13, 2020 13:35
stephentoub
stephentoub reviewed Jul 13, 2020
View reviewed changes
stephentoub
stephentoub reviewed Jul 13, 2020
View reviewed changes
src/libraries/Common/src/Interop/Windows/WinHttp/Interop.winhttp_types.cs Outdated
Copy link
Member

@stephentoub stephentoub Jul 13, 2020

Choose a reason for hiding this comment

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

Why are these commented out? Shouldn't they either be deleted or uncommented? Or does the tooling pay attention to commented out SuppressMessage attributes?

Copy link
Contributor Author

@aik-jahoda aik-jahoda Jul 14, 2020

Choose a reason for hiding this comment

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

Yes, it is official way how to suppress report for the next line.

Copy link
Member

@danmoseley danmoseley Jul 15, 2020

Choose a reason for hiding this comment

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

I wonder how we will remember to not remove these as dead code! Would this also be recognized?

// [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="It is property descriptor, not secret value.")]      // Commented line recognized by credscan tool

?

Copy link
Member

@stephentoub stephentoub Jul 15, 2020

Choose a reason for hiding this comment

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

My preference would be actually running the analyzer in dotnet/runtime.

Copy link
Member

@danmoseley danmoseley Jul 15, 2020

Choose a reason for hiding this comment

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

Can you clarify, I thought that's what @aik-jahoda is doing.

Copy link
Member

@stephentoub stephentoub Jul 15, 2020

Choose a reason for hiding this comment

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

Very few direct check-ins are made to the private mirror. It makes no sense to me to run analyzers over that source that we don't also run as part of the public repo, where 99.999% of all commits come from. Doing that only causes additional dev work and headaches, and it's caused problems as well, where mirroring fails because the already merged commits then fail the subsequent pre-merge validation. This also isn't a one-time thing; PRs like this have been submitted at multiple times in the past in order to mop up from such analyzers that are being run late. And to your question about not deleting code as dead/stale, this exact mechanism would address that. So I don't understand the concerns or pushbavk. It seems like a no brainer to me. What am I not comprehending?

Not all files are in the build

And what percentage of the changes in this PR are to such files? I'm not saying it would prevent all possible problems. You're saying it's useless if it's not 100% false negatives? I highly doubt the existing tooling even gets close to that.

Copy link
Member

@ManickaP ManickaP Jul 15, 2020 •
edited by danmoseley
Loading

Choose a reason for hiding this comment

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

Just info about the tool and public PRs, it cannot be run out in the open: dotnet/arcade#4663

But there's an open issue https://github.com/dotnet/core-eng/issues/5747 about running it on public PRs.

Though I'm not what it will be good for without the results 🤣

Copy link
Member

@stephentoub stephentoub Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I don't buy the answer in that issue, though. We run analyzers that validate, for example, SHA1 isn't being used, and that's an "SDL static verification tool".

Copy link
Contributor Author

@aik-jahoda aik-jahoda Jul 20, 2020

Choose a reason for hiding this comment

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

@danmosemsft, are you ok to merge this PR? I think we currently don't have a better option how to suppress the bugs reported by the tool.

Copy link
Member

@bartonjs bartonjs Jul 20, 2020

Choose a reason for hiding this comment

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

The changes look OK to me, modulo the large conceptual problem of how there's a gap between checkins and analysis. I'm withholding a checkmark to let @danmosemsft decide if we want to block this on the conceptual problem or not.

@stephentoub stephentoub requested a review from bartonjs July 13, 2020 16:48
bartonjs
bartonjs reviewed Jul 13, 2020
View reviewed changes
bartonjs
bartonjs reviewed Jul 13, 2020
View reviewed changes
@aik-jahoda
Copy link
Contributor Author

aik-jahoda commented Jul 14, 2020

@bartonjs, @stephentoub thanks for your comments. I have replied, can you let me know your thoughts please?

@aik-jahoda
Copy link
Contributor Author

aik-jahoda commented Jul 14, 2020 •
edited
Loading

There is an example how it would look like if we have // [SuppressMessage ...] only: fac0d23

@ManickaP
Copy link
Member

ManickaP commented Jul 14, 2020

There is an example how it would look like if we have // [SuppressMessage ...] only: fac0d23

Personally, I like it much better 👍 It's clearer what it's for than the TestSecretHelper magic.

Although I think that, for most of the test files, we could have just leave the whole file suppressed in CredScanSuppressions.json.

@aik-jahoda aik-jahoda force-pushed the jajahoda/credscan1 branch from 2295353 to 186991e Compare July 15, 2020 14:16
@aik-jahoda
Copy link
Contributor Author

aik-jahoda commented Jul 15, 2020

I reverted it to the // [SuppressMessage...] approach.
There are rules for generic cases in .config/CredScanSuppressions.json which remove a lot of false positives. The rest is about add a suppress message or change credentials to credentials defined in .config/CredScanSuppressions.json

cc @ManickaP, @bartonjs, @stephentoub

bartonjs
bartonjs reviewed Jul 15, 2020
View reviewed changes
danmoseley
danmoseley reviewed Jul 15, 2020
View reviewed changes
aik-jahoda and others added 2 commits July 16, 2020 15:56
@aik-jahoda @bartonjs
Apply suggestions from code review
3fe5e24 
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
Revert passwords literals
921d9f0
@aik-jahoda aik-jahoda force-pushed the jajahoda/credscan1 branch from 9f53327 to 921d9f0 Compare July 21, 2020 09:08
danmoseley
danmoseley reviewed Jul 21, 2020
View reviewed changes
danmoseley
danmoseley approved these changes Jul 21, 2020
View reviewed changes
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

I think this is OK -- whether or not we can move this credscan checking "upstream" or not, it makes sense to get it clean now as part of the larger effort, and minimally affects the product code.

@aik-jahoda aik-jahoda merged commit 5c274f6 into dotnet:master Jul 23, 2020
@aik-jahoda aik-jahoda deleted the jajahoda/credscan1 branch July 23, 2020 09:59
This was referenced Aug 6, 2020
Merged
Closed
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@bartonjs @Jacksondr5
Suppress credscan false positives (dotnet#38026)
c117b87 
* Suppress initial cred issues

* Another bunch of supresses

* Clean up

* Another bunch of supresses

* Revert to suppression messages

* Clean up

* Apply suggestions from code review

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>

* Revert passwords literals

* Fix suppression justification comment

Co-authored-by: Jan Jahoda <jajahoda@.microsoft.com>
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@danmoseley danmoseley danmoseley approved these changes

@bartonjs bartonjs bartonjs left review comments

@ManickaP ManickaP ManickaP left review comments

Assignees

No one assigned

Labels

area-Infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aik-jahoda @Dotnet-GitSync-Bot @ManickaP @stephentoub @danmoseley @bartonjs