search

microsoft / binskim

A binary static analysis tool that provides security and correctness results for Windows Portable Executable and *nix ELF binary formats
Other
776 stars 157 forks source link

Rename `BA2026.EnableAdditionalSdlSecurityChecks` to `BA2026.EnableMicrosoftCompilerSdlSwitch` #584

Closed michaelcfanning closed 2 years ago

michaelcfanning commented 2 years ago

The existing rule name sounds extremely generic. The new rule name is intended to make it a bit clearer that this rule is enforcing a specific Microsoft compiler security option, its '/sdl' switch.

michaelcfanning commented 2 years ago

@shaopeng-gh. @marmegh, easy issue to pick up. If we wanted to be scrupulous, we could add the old rule name as a deprecated friendly name. This might require a plug-in architecture change to BinSkim, glad to discuss this with you if you'd like to pursue that.

marmegh commented 2 years ago

@michaelcfanning, would this be strictly updating the friendly name or would we also be looking to rename the class?

michaelcfanning commented 2 years ago

We would want to comprehensively update every occurrence of the old name to the new, including class names file names, source code comments, etc. :)

In theory, we would eventually retain a single reference to the old name, in some new property hanging off a skimmer, DeprecatedRuleNames. This property would be consumed by the logger in order to emit the deprecated rule name when it creates SARIF rule table entries. This scaffolding does not exist, and we should first look at the SARIF SDK (specifically, the Sarif.Driver project) to think about adding it. So let's separate the two workstreams (and I will follow up with you offline on the SARIF SDK possibility. I think the simple rule id rename s/be pretty straightforward).