8000 Fix credentials filtering with no allowList by lunt7 · Pull Request #401 · solokeys/solo1 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix cred 8000 entials filtering with no allowList #401

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix credentials filtering with no allowList #401

wants to merge 1 commit into from

Conversation

lunt7
Copy link
@lunt7 lunt7 commented Mar 23, 2020

getAssertionState.creds[ALLOW_LIST_MAX_SIZE] has space for
ALLOW_LIST_MAX_SIZE credentials but we are only filling it up to
ALLOW_LIST_MAX_SIZE - 1

@conorpp
Copy link
Member
conorpp commented Mar 27, 2020

Since C arrays start at 0, I believe GA->creds[ALLOW_LIST_MAX_SIZE - 1]; is the last element?

@jolo1581
Copy link
Contributor
jolo1581 commented Mar 29, 2020 •
edited
Loading

@conorpp you are right n-1 ist the last element of the array. But in this case this is an error exception if your counter is outside array size. So n-1 is valid and n not.

Also it is not a good style asking for equal. Better it would be:

if (count > ALLOW_LIST_MAX_SIZE-1)

or

if (count >= ALLOW_LIST_MAX_SIZE)

So If @lunt7 would correct this to one of the example above and it would be right.

Greetz Jan

@lunt7
Fix array bound checking in credentials filtering with no allowList
0955619 
count is an index into the creds array in getAssertionState, and it is checked against the array bounds before data are copied.
The array contains space for ALLOW_LIST_MAX_SIZE entries but we are only filling it up to ALLOW_LIST_MAX_SIZE - 1 since the loop terminates early.
@lunt7 lunt7 force-pushed the fix_filter_credentials branch from 8913f17 to 0955619 Compare March 30, 2020 18:51
@lunt7
Copy link
Author
lunt7 commented Mar 30, 2020

There is a similar condition checking in save_credential_list(), so I decided to go with the first one to be consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lunt7 @conorpp @jolo1581
0