Fix use-after-free of std::list iterator in FormFiller::UndoAutofill [chromium/src : main]
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
size_ -= fill_operation->erase(field_id);This would still crash if we call it after calling the erase below
Related details
- Christoph Schwering
- Gianmarco Picarella
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gianmarco Picarella (Gerrit)
Gianmarco Picarella added 1 comment![]( "Open in Gerrit")
size_ -= fill_operation->erase(field_id);This would still crash if we call it after calling the erase below
True, I am wondering what could be the best approach for this case.
What if we use a boolean to inform the caller that the passed iterator got invalidated?
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna added 1 comment![]( "Open in Gerrit")
size_ -= fill_operation->erase(field_id);Gianmarco PicarellaThis would still crash if we call it after calling the erase below
True, I am wondering what could be the best approach for this case.
What if we use a boolean to inform the caller that the passed iterator got invalidated?
What about changing the function to `EraseFieldFillingEntries(base::flat_set<FieldGlobaId>)`? We would then call erase only at the end.
Related details
- Christoph Schwering
- Gianmarco Picarella
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gianmarco Picarella (Gerrit)
Gianmarco Picarella added 1 comment![]( "Open in Gerrit")
size_ -= fill_operation->erase(field_id);Gianmarco PicarellaThis would still crash if we call it after calling the erase below
Jihad HannaTrue, I am wondering what could be the best approach for this case.
What if we use a boolean to inform the caller that the passed iterator got invalidated?
What about changing the function to `EraseFieldFillingEntries(base::flat_set<FieldGlobaId>)`? We would then call erase only at the end.
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna voted and added 9 comments![]( "Open in Gerrit")
Votes added by Jihad Hanna
| Code-Review | +1 |
9 comments
size_ -= fill_operation->erase(field_id);Gianmarco PicarellaThis would still crash if we call it after calling the erase below
Jihad HannaTrue, I am wondering what could be the best approach for this case.
What if we use a boolean to inform the caller that the passed iterator got invalidated?
Gianmarco PicarellaWhat about changing the function to `EraseFieldFillingEntries(base::flat_set<FieldGlobaId>)`? We would then call erase only at the end.
Yes, I agree!
Thank you :)
if (entry_iter->empty()) {
history_.erase(entry_iter);
}I understand that the fact that the set imposes uniqueness resolves the bug at hand, but I'd rather we move this outside the loop and till the end.
// Multiple fields within a single form may have the same FieldGlobalId. Tobackticks
// Multiple fields within a single form may have the same FieldGlobalId. To // FormAutofillHistory::AddFormFillingEntry(...) silently collapses fieldsbackticks
// FormAutofillHistory::AddFormFillingEntry(...) silently collapses fieldsremove
// Multiple fields within a single form may have the same FieldGlobalId. To
// prevent redundant deletion attempts from the autofill history, we filter
// for unique IDs before processing the list. This is needed because
// FormAutofillHistory::AddFormFillingEntry(...) silently collapses fields
// with duplicate FieldGlobalId.I think this comment is confusing, the problem we're after is UAF caused by deleting the entry then accessing it again using duplicate IDs.
Related details
- Christoph Schwering
- Gianmarco Picarella
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gianmarco Picarella (Gerrit)
Gianmarco Picarella added 7 comments![]( "Open in Gerrit")
if (entry_iter->empty()) {
history_.erase(entry_iter);
}I understand that the fact that the set imposes uniqueness resolves the bug at hand, but I'd rather we move this outside the loop and till the end.
Okay!
// Multiple fields within a single form may have the same FieldGlobalId. To // Multiple fields within a single form may have the same FieldGlobalId. ToGianmarco Picarellabackticks
Done
// FormAutofillHistory::AddFormFillingEntry(...) silently collapses fieldsGianmarco Picarellaremove
Done
// FormAutofillHistory::AddFormFillingEntry(...) silently collapses fieldsGianmarco Picarellabackticks
Done
// Multiple fields within a single form may have the same FieldGlobalId. To
// prevent redundant deletion attempts from the autofill history, we filter
// for unique IDs before processing the list. This is needed because
// FormAutofillHistory::AddFormFillingEntry(...) silently collapses fields
// with duplicate FieldGlobalId.I think this comment is confusing, the problem we're after is UAF caused by deleting the entry then accessing it again using duplicate IDs.
Does it look clear now?
// with duplicate FieldGlobalId.Gianmarco Picarellabackticks
Done
Related details
- Christoph Schwering
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christoph Schwering (Gerrit)
Christoph Schwering added 4 comments![]( "Open in Gerrit")
void EraseFieldFillingEntries(const base::flat_set<FieldGlobalId>& fields_id);nit: `field_ids`?
if (entry_iter->empty()) {
history_.erase(entry_iter);
}Gianmarco PicarellaI understand that the fact that the set imposes uniqueness resolves the bug at hand, but I'd rather we move this outside the loop and till the end.
Okay!
For both the PS3 and PS4 version I don't see yet why it works:
PS3: Why is `CHECK(entry_iter != history_.end());` guaranteed? It depends on the caller, doesn't it?
PS4: What prevents us from having duplicates in `operations` and thus double frees?
Shouldn't we just do `std::erase_if(history_, [](const FormFillingEntry& entry){ return entry.empty(); });`?
// FormAutofillHistory::AddFormFillingEntry(...) silently collapses fieldsGianmarco Picarellaremove
Done
Btw I'm not aware of a rule to put every code in backticks. I think it often just adds noise.
// `FieldGlobalId` is encountered, causing an usage-after-free crash. Thisnit: remove, because the crash is not guaranteed (I think) and among the better outcomes.
Related details
- Gianmarco Picarella
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |