ews: enforce Send-As permission on client-supplied From/Sender - #316
Open
nourdineb-ops wants to merge 1 commit into
Open
ews: enforce Send-As permission on client-supplied From/Sender#316nourdineb-ops wants to merge 1 commit into
nourdineb-ops wants to merge 1 commit into
Conversation
toContent() has always written whatever identity a client supplied via
a message's From/Sender field straight into PR_SENT_REPRESENTING_* and
PR_SENDER_*, for both CreateItem's SaveOnly (draft) and SendOnly/
SendAndSaveCopy (actual send) paths - with no authorization check of
its own. Real Exchange, and gromox's own zcore (zs_submitmessage())
and emsmdb paths, instead require frightsGromoxSendAs or full store
ownership on the claimed identity's mailbox before honoring it - EWS
was the only one of gromox's three client-facing paths missing this.
Traced empirically on a live deployment: grommunio-web correctly
denies/grants based on this permission (addUserStore() -> a genuine
MAPI store-open, checked server-side), while an EWS CreateItem with an
arbitrary From currently goes through unchecked - confirmed via raw
delivered headers showing a clean, unauthorized identity with no
rejection anywhere in the pipeline.
Root cause: a prior commit ("ews: implement ReplyToItem/.../and
respect client-supplied From on save") intentionally started trusting
a client-supplied From to fix a real bug (drafts from a secondary
alias reverting to the primary identity) and explicitly modeled this
on grommunio-web's PR_SENT_REPRESENTING_* ??= PR_SENDER_* pattern -
but only copied the fallback-default half of that pattern, not the
permission check that makes it safe in the webapp
(class.operations.php's addUserStore()/openMessageStore()).
Adds EWSContext::checkSendAs(), mirroring zs_submitmessage()'s check,
called from CreateItem right after toContent() populates
PR_SENT_REPRESENTING_EMAIL_ADDRESS - applies uniformly to drafts and
actual sends, and to the reply/forward wrapper types alongside plain
messages, since they all flow through the same toContent() call.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Just checking in - any questions on this one, or anything else you'd like me to test/expand on before it gets a look? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sorry, missed something in #286 that I should have caught then. Found and fixed a permission gap in EWS's
CreateItemwhile looking into something unrelated (a "Send As" convenience script on our own deployment).toContent()writes whatever identity a client supplies via a message's From/Sender field straight intoPR_SENT_REPRESENTING_*/PR_SENDER_*, for bothSaveOnlyandSendOnly/SendAndSaveCopy- with no authorization check.Root cause: #286 ("respect client-supplied From on save") intentionally started trusting a client-supplied From to fix a real bug - a draft saved from a secondary alias reverting to the primary identity - and explicitly modeled this on grommunio-web's
PR_SENT_REPRESENTING_* ??= PR_SENDER_*pattern (class.operations.php). It only carried over the fallback-default half of that pattern, not the permission check that makes it safe there.Fix adds
EWSContext::checkSendAs(), mirroringzs_submitmessage()'s check (frightsGromoxSendAsor full store ownership on the claimed identity's mailbox), called fromCreateItemright aftertoContent()populatesPR_SENT_REPRESENTING_EMAIL_ADDRESS- applies uniformly to drafts and actual sends, and to the reply/forward wrapper types alongside plain messages.Tested against a live deployment: granted a real
frightsGromoxSendAsgrant between two test mailboxes, confirmed aCreateItemwith the granted identity still succeeds and correctly resolves to it, confirmed the same request with an ungranted identity now getsErrorAccessDenied, and confirmed a plain message with no From override is unaffected.