Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cap sendInputEvent text length at n-1 #27827

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Conversation

nornagon
Copy link
Member

Description of Change

This fixes two bugs with sendInputEvent, detected by ASan.

  1. The call to memset was not clearing the full buffer, as the size of each
    element of the array is 2 bytes, not 1.
  2. This buffer is at some later point passed to string() as a raw char16
    pointer, which causes the length of the string to be computed by seeking for
    a null terminator. However, this results in an out-of-bounds access as we
    often fill all 4 characters of both text and unmodified_text with
    non-null values.

It looks like this code was inspired by
BuildCharEvent
in content/renderer/pepper/event_conversion.cc, so I've updated this to be a
little closer to that code. I think the upstream code also has this issue but
gets away with it because it doesn't set unmodified_text, so there's always a
\0 after text due to the struct layout.

I'm not sure if we actually need unmodified_text---the pepper converter does
not fill it in.

Further, we fill text from keyCode, which is a bit of an odd choice. The
value is supposed to represent the text that should be inserted, which for the
key code Escape is not "Esca". It's not clear to me what the "right" path
forward is here, but this at least prevents the OOB access.

Checklist

Release Notes

Notes: Fixed an out-of-bounds access in WebContents.sendInputEvent.

@nornagon nornagon added semver/patch backwards-compatible bug fixes target/10-x-y labels Feb 19, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 19, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 20, 2021
@korakot25

This comment has been minimized.

@jkleinsc jkleinsc merged commit 912c9c2 into master Feb 22, 2021
@jkleinsc jkleinsc deleted the cap-sendinputevent branch February 22, 2021 15:39
@release-clerk
Copy link

release-clerk bot commented Feb 22, 2021

Release Notes Persisted

Fixed an out-of-bounds access in WebContents.sendInputEvent.

@trop
Copy link
Contributor

trop bot commented Feb 22, 2021

I have automatically backported this PR to "11-x-y", please check out #27853

@trop
Copy link
Contributor

trop bot commented Feb 22, 2021

I have automatically backported this PR to "10-x-y", please check out #27854

@trop
Copy link
Contributor

trop bot commented Feb 22, 2021

I have automatically backported this PR to "12-x-y", please check out #27855

@attritionorg
Copy link

@nornagon Does '2' as described represent a security risk?

@nornagon
Copy link
Member Author

nornagon commented Jun 2, 2021

@attritionorg technically I suppose yes, but I'd rank it as very difficult or impossible to exploit, as the OOB access is only a couple of bytes wide, and into other data on the stack that is not very interesting. Further, this is only in the main process, which is ostensibly trusted code.

@attritionorg
Copy link

@nornagon Great, thank you for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants