commit b9d30f1da7c5975ec3fbee52dce92d02b57a77a8
author Guillaume Jenkins <gujen@google.com> Tue Oct 04 14:42:53 2022
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Oct 04 14:42:53 2022
tree d595560f0d02e67478c60c65a8a9d7c373ee304c
parent 40a95ab85eab454762979e59f07e57f226be54cd

[UserNotes] Fix UAF in OnNoteCreationDone

UserNoteService was passing a UserNote pointer to the storage layer,
which was making async calls on a separate sequence and dereferencing
the pointer there. This was a potential UAF if the UserNote was
destroyed in the service while the async call was running on the
sequence.

Ideally, the fix would have been to pass a WeakPtr and validate it on
the sequence, but UserNote already has a WeakPtrFactory that creates
SafeRefs for the UserNoteInstance objects, so it is already bound to the
main thread. As such, any WeakPtr it produces cannot be dereferenced on
another sequence.

The proposed fix is thus to create a clone of the UserNote, and pass
that to the storage layer and its sequence. Because AsyncCall() uses
the move constructor to forward args to the other sequence, and the
move constructor of UserNote has been deleted by its WeakPtrFactory,
the clone needs to be a unique_ptr. An alternative would have been to
pass the values of the UserNote properties as function parameters, but
there are up to 8, which would have made the code hard to read and maintain.

Bug: 1363583
Change-Id: I96fed460901cced5148f0c0fb44a7fc21b7618a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3910071
Reviewed-by: Yuheng Huang <yuhengh@chromium.org>
Commit-Queue: Guillaume Jenkins <gujen@google.com>
Cr-Commit-Position: refs/heads/main@{#1054708}