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

Bugfix: command injection vulnerabilities #888

Merged
merged 10 commits into from
May 10, 2021
Merged

Bugfix: command injection vulnerabilities #888

merged 10 commits into from
May 10, 2021

Conversation

billz
Copy link
Member

@billz billz commented May 2, 2021

No description provided.

Copy link
Contributor

@glaszig glaszig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(deleted)

@billz billz added the security label May 3, 2021
@billz
Copy link
Member Author

billz commented May 3, 2021

@glaszig any changes?

@glaszig
Copy link
Contributor

glaszig commented May 4, 2021

yes, see comments in code.

@billz
Copy link
Member Author

billz commented May 5, 2021

@glaszig feel free to revert any commits or push other changes.

@omriinbar
Copy link

Any progress on this?

@glaszig
Copy link
Contributor

glaszig commented May 6, 2021

reverted the get/post thing. confirmed that unicode wifi name encoding in qr code is broken with this changed. i'll try to figure this out.

@billz
Copy link
Member Author

billz commented May 7, 2021

Unicode chars in escapeshellarg are supposedly handled by php locale. however, according to the IEEE 802.11i-2004 spec:

A pass-phrase is a sequence of between 8 and 63 ASCII-encoded characters.

with another footnote:

Each character in the pass-phrase must have an encoding in the range of 32 to 126 (decimal), inclusive. (IEEE Std. 802.11i-2004, Annex H.4.1)

which suggests we should skip unicode altogether and validate ASCII.

@glaszig
Copy link
Contributor

glaszig commented May 7, 2021

i'm talking about the ssid, though. not the passphrase ;)

@glaszig
Copy link
Contributor

glaszig commented May 8, 2021

alright. i tested multibyte functionality with different locales and escapeshellarg. it simply does not work (in this case).
doing so i think to have identified the root-cause of the problem: double quotes. my original implementation of mb_escapeshellarg always returns double-quoted values while the original upstream version returns single-quoted value on non-windows systems.

thus i adopted the original upstream version of the function. imo this is safe as anything inside single-quoted shell strings - like $(...) will not be evaluated.

this way, the qr code works with unicode ssid's again.

if you guys will approve, i'm done with this one.

@glaszig
Copy link
Contributor

glaszig commented May 8, 2021

i also added a margin to the qr code because scanning didn't work on dark background (dark theme).

@billz
Copy link
Member Author

billz commented May 10, 2021

tested multibyte ssids and confirmed mb_escapeshellarg does what it should now. if locales are not working, in this case, we can safely remove this include. I'd say we're good to merge. @glaszig @omriinbar thanks

@billz billz merged commit 061d01c into master May 10, 2021
@billz billz deleted the bugfix/security branch May 10, 2021 08:55
@eugenebmx
Copy link

@billz hey, the fix in this PR is not complete. Please lmk how can I provide you with the details.

@RaspAP RaspAP locked as resolved and limited conversation to collaborators Jun 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants