-
Notifications
You must be signed in to change notification settings - Fork 261
Strip confidential information from logs on "error" level (minimal version) #2497
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow)
Pal/src/host/Linux-SGX/db_exception.c, line 225 at r1 (raw file):
event_num != PAL_EVENT_INTERRUPTED) { log_error("*** Unexpected exception occurred inside PAL at RIP = +0x%08lx! ***", uc->rip - (uintptr_t)TEXT_START);
This one is actually useful and it's just an offset in PAL binary. Is that really confidential?
Pal/src/host/Linux-SGX/db_exception.c, line 234 at r1 (raw file):
} log_debug("rax: 0x%08lx rcx: 0x%08lx rdx: 0x%08lx rbx: 0x%08lx\n"
IMO you can just remove this, it's useless anyway (but not blocking).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski)
Pal/src/host/Linux-SGX/db_exception.c, line 225 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This one is actually useful and it's just an offset in PAL binary. Is that really confidential?
Tbh I'm not sure. I thought it's not really useful and just stripped it.
The problem is that it's much more granular than what host sees normally (byte-level granularity of RIP).
Pal/src/host/Linux-SGX/db_exception.c, line 234 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
IMO you can just remove this, it's useless anyway (but not blocking).
I'd prefer to leave this, but no strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, all discussions resolved, "fixup! " found in commit messages' one-liners
Pal/src/host/Linux-SGX/db_exception.c, line 225 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Tbh I'm not sure. I thought it's not really useful and just stripped it.
The problem is that it's much more granular than what host sees normally (byte-level granularity of RIP).
I agree that it's not confidential. Graphene shouldn't throw this error anyway (only on Graphene bugs), and I don't see how it can facilitate attacks. So the current version looks fine to me.
Pal/src/host/Linux-SGX/db_exception.c, line 234 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I'd prefer to leave this, but no strong feelings.
I also find this one useless. Never once these reg values helped me :)
Signed-off-by: Michał Kowalczyk <mkow@invisiblethingslab.com>
e9e37d7
to
754d4e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @boryspoplawski and @dimakuv)
Pal/src/host/Linux-SGX/db_exception.c, line 234 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I also find this one useless. Never once these reg values helped me :)
Ok, let's drop them then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
Pal/src/host/Linux-SGX/db_exception.c, line 225 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree that it's not confidential. Graphene shouldn't throw this error anyway (only on Graphene bugs), and I don't see how it can facilitate attacks. So the current version looks fine to me.
@dimakuv the trick here is that malicious host can trigger this at will, because it can inject an event value as long as it's valid one. This will change once I rewrite this :)
Description of the changes
This is a slightly simplified version of #2468 (please see its description for more details).
I talked with @boryspoplawski and @pwmarcz and decided that the proper solution for this should be making
log_*()
censor format string replace points, but then I looked at thevfprintfmt()
code and gave up. So, for now let's at least fix the most dangerous messages.There are a lot of changes to logging in protected files, because it seems that most of the messages there are actually warnings (i.e. Graphene continues to work correctly after printing them) and there also emit a lot of information.
This change is