syscalls: Sanitize all pointers #606
Conversation
Unit Test Results7 787 tests +46 7 054 ✅ +28 43m 24s ⏱️ + 3m 41s For more details on these failures, see this check. Results for commit a3d15c4. ± Comparison against base commit 84b66c2. ♻️ This comment has been updated with latest results. |
DONE: RTOS-939
agkaminski
force-pushed
the
agkaminski/rtos-939
branch
from
0153d07
to
a3d15c4
Compare
| if (vm_mapBelongs(proc, v, sizeof(v)) < 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| for (i = 0;; ++i) { | ||
| if (vm_mapBelongs(proc, &v[i], sizeof(v)) < 0) { | ||
| return -1; | ||
| } |
| @@ -1120,6 +1122,32 @@ int vm_mapBelongs(const struct _process_t *proc, const void *ptr, size_t size) | |||
| } | |||
|
|
|||
|
|
|||
| int vm_mapStringBelongs(const struct _process_t *proc, const char *str) | |||
There was a problem hiding this comment.
This check has one more vulnerability than vm_mapBelongs has. User can also alter the string post NUL byte check. We either have to copy the string or pass the string with length found by this function.
| @@ -173,52 +175,119 @@ int syscalls_release(void *ustack) | |||
| } | |||
|
|
|||
|
|
|||
| static int syscalls_sanitizeVector(process_t *proc, char **v) | |||
There was a problem hiding this comment.
On top of vulnerabilities of vm_mapStringBelongs and vm_mapStringBelongs, this function is also vulnerable to user changing the elements of vector. Unfortunatelly, I think that there's no way around copying the vector, but a FIXME note should be sufficient for now.
|
Probably beyond the scope of this PR, but it would be nice to be resiliant to mov sp,0
int 0x80 |
| @@ -1120,6 +1122,32 @@ int vm_mapBelongs(const struct _process_t *proc, const void *ptr, size_t size) | |||
| } | |||
|
|
|||
|
|
|||
| int vm_mapStringBelongs(const struct _process_t *proc, const char *str) | |||
There was a problem hiding this comment.
Shouldn't there be a fast return for str == NULL or at least handle that per-syscall? To avoid arithmetic on NULLs and unnecessary locks.
DONE: RTOS-939
Description
Motivation and Context
Fixes phoenix-rtos/phoenix-rtos-project#976
Types of changes
How Has This Been Tested?
Checklist:
Special treatment