Mark all stormlib functions that are readonly safe as readonly safe (SYN-4905) #3402
Conversation
vEpiphyte
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3402 +/- ##
==========================================
- Coverage 97.38% 97.30% -0.09%
==========================================
Files 233 233
Lines 47899 48124 +225
==========================================
+ Hits 46646 46825 +179
- Misses 1253 1299 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
vEpiphyte
changed the title
vEpiphyte
requested review from
invisig0th,
MichaelSquires,
Cisphyx and
mikemoritz
| async def _getCellInfo(self): | ||
| if not self.runt.isAdmin(): | ||
| mesg = '$lib.cell.getCellInfo() requires admin privs.' | ||
| raise s_exc.AuthDeny(mesg=mesg, user=self.runt.user.iden, username=self.runt.user.name) | ||
| return await self.runt.snap.core.getCellInfo() | ||
|
|
||
| @s_stormtypes.stormfunc(readonly=True) |
There was a problem hiding this comment.
Is there any concern about returning admin restricted info even if it doesn't modify anything?
There was a problem hiding this comment.
I'm not aware of any concerns there. An unprivileged execution context would still not be able to access this information.
One scenario where this could be used is in an extended HTTP API handler. A handler setup to run as the owner ( an admin ) in readonly mode could then access this information and that should be okay IMO; since that would be an opt-in behavior by the org/individuals configuring their Cortex.
| @@ -653,10 +653,12 @@ def getObjLocals(self): | |||
| 'validate': self.validateBundle, | |||
| } | |||
|
|
|||
| @s_stormtypes.stormfunc(readonly=True) | |||
There was a problem hiding this comment.
The semafork is limited to a fixed number of forks, right? Could this be used to fork bomb?
There was a problem hiding this comment.
fork bomb; no. We limit the # of concurrent semafork calls such that the forkpool always has reserved workers for parsing storm queries.
Bad input here ( bad meaning the stix validation to go out to lunch ) would be a DOS regardless of readonly status.
There was a problem hiding this comment.
Per discussion we'll leave this as is ( with the readonly=true marking ).
No description provided.