Closed Bug 1611160 Opened 2 years ago Closed 1 year ago

Add linter rule to block calling Principal->GetURI()

Categories

(Firefox Build System :: Source Code Analysis, task, P2)

task

Tracking

(firefox75 fixed)

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: ckerschb, Assigned: sstreich)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

Within Bug we would like refactor the Principal API so to ask the Principal security questions but do not query the URI of a principal and make security decisions based on that URI.

Wile Bug 1577165 is on the way we should somehow ensure to not introduce new calls to principal->GetURI.

Sylvestre, where would I add such a rule? Here is what we would like to prevent being called:
https://searchfox.org/mozilla-central/rev/e878e5b81bb319c141900ce9cfcde732df5c8449/caps/nsIPrincipal.idl#78

Flags: needinfo?(sledru)

So, when the class of the object if nsIPrincipal (or a child) and call GetURI, we want to break the CI ?

Is there a backlog of this pattern in the code base?

Flags: needinfo?(sledru)

(In reply to Sylvestre Ledru [:Sylvestre] from comment #2)

Is there a backlog of this pattern in the code base?

Yes, there are a lot of callsites left currently in the codebase, see:
https://searchfox.org/mozilla-central/search?q=symbol:_ZN12nsIPrincipal6GetURIEPP6nsIURI&redirect=false

I think it's more than just breaking the CI, right?

Flags: needinfo?(sledru)

So, we want to warn for new usage at review phase but not deal with previous cases (just like with clang-tidy), right?

Flags: needinfo?(sledru)

(In reply to Sylvestre Ledru [:Sylvestre] from comment #4)

So, we want to warn for new usage at review phase but not deal with previous cases (just like with clang-tidy), right?

Yes, that is correct! Is that possible? If so, can you help me set that up?

Flags: needinfo?(sledru)

Yeah, Andi will explain how it works (or do it for you)

Flags: needinfo?(sledru) → needinfo?(bpostelnicu)

In order to have this as a checker we need to develop a matcher that would match CallExpr and fro the CallExpr we will get the CXXMethodDecl and determine if it's parent is QualType - Principal.
Now I see myself needed to ask how many occurrences of this kind we have in the code and If we want to change them before landing the checker or not. This matters on the way how we want to post clang-diagnostic-message, if we push the checker in tree and we consider the diagnostic to be error and we don't fix the issues prior than we will bust the build otherwise we are fine. But if we want to push the checker but we don't have time or willingness to fix the in-tree issues by the time we push we should emit warnings as clang-diagnostic-message.
Please note no matter what we emit these issues will be detected during review-phase by the bot.

Component: DOM: Security → Source Code Analysis
Flags: needinfo?(bpostelnicu)
Product: Core → Firefox Build System
Version: unspecified → Trunk
Assignee: ckerschb → sstreich
Priority: -- → P2
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb60b22ee661
Add Clang Plugin for nsIPrincipal r=ckerschb,andi
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Regressions: 1638576
You need to log in before you can comment on or make changes to this bug.