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

Potential security issue #1616

Closed
zidingz opened this issue Sep 24, 2021 · 15 comments · Fixed by #1619
Closed

Potential security issue #1616

zidingz opened this issue Sep 24, 2021 · 15 comments · Fixed by #1619
Assignees

Comments

@zidingz
Copy link

zidingz commented Sep 24, 2021

Hey there!

I belong to an open source security research community, and a member (@srikanthprathi) has found an issue, but doesn’t know the best way to disclose it.

If not a hassle, might you kindly add a SECURITY.md file with an email, or another contact method? GitHub recommends this best practice to ensure security issues are responsibly disclosed, and it would serve as a simple instruction for security researchers in the future.

Thank you for your consideration, and I look forward to hearing from you!

(cc @huntr-helper)

@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 24, 2021

I might be wrong here, but seems that you are posting almost the same thing in many repos?
You can find the repo owners email in the git commits - SECURITY.md sure has it's place, but it can also be a hindrance, and out of date if just created. I will be against it in this repo/project.

@JamieSlome
Copy link

Hello @NiKiZe - we request a SECURITY.md, so we can be sure that the person receiving the report has explicit write permissions to the repository.

We reach out via Github Issue in this way, as we want to follow a responsible disclosure process and ensure we send the security concerns to the right, and authorised individuals.

Let me know if you have any questions or further thoughts! 😊

@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 24, 2021

That documentation is only valid if keept up to date, which it might not be even if it is there.
Not sure what security has anything to do with write permission.

Now 3 accounts are involved. Seems weird-ish.

@crankyoldgit
Copy link
Owner

I belong to an open source security research community

What is the name of the community? Got a link to something?

Hello @NiKiZe - we request a SECURITY.md, so we can be sure that the person receiving the report has explicit write permissions to the repository.

@JamieSlome I definitely have explicit write permission (as the owner of the repo), @NiKiZe has write access too.

Let me know if you have any questions or further thoughts! 😊

This is a completely open project. Feel free to create a new issue (or reuse this one) for any security problems.
I've created a draft Security.md in PR #1617 Please have a read. It expresses my views I think.
Unless there is something implicitly confidential/sensitive in nature, I'm fine with it being public knowledge with in the repo and in the open so to speak.
e.g. If you're dealing with an undisclosed confidential vuln in some component we are using etc., or we are the root cause of some RCE in a major project (e.g. Tasmota), then sure then lets keep it under-wraps etc. If it's a vuln in one of our examples etc, then a normal bug is fine etc.

That said, sure, we'd love to know of & fix the vuln before it is widely publicised etc. e.g. Before you go making announcements etc.

FWIW, I've previously worked at a Nation-level CERT, and Multi-national IT/Internet company on their IT Security teams. Not bragging, just saying I'm fully aware of the things, topics, & issues involved and willing to assist etc.

GPG key is available if required, but nothing in this project/library really warrants that level of paranoia. (plus I'd have to dust off the passphrase ;-)

Oh, and thanks for following "Responsible Disclosure" guidelines. Appreciated. Also happy to give appropriate attribution too.

@JamieSlome
Copy link

@crankyoldgit:

What is the name of the community? Got a link to something?

https://huntr.dev

@JamieSlome I definitely have explicit write permission (as the owner of the repo), @NiKiZe has write access too.

https://huntr.dev/bounties/4da00a75-50dc-458b-acc6-cc216e1c854a/ is the actual report. We don't share even this when we create the issue, as it can be seen as breaking GitHub's Guidelines and Community rules (i.e. advertising).

I've created a draft Security.md in PR #1617 Please have a read. It expresses my views I think.

Dropped a review on it!

@crankyoldgit
Copy link
Owner

https://huntr.dev/bounties/4da00a75-50dc-458b-acc6-cc216e1c854a/ is the actual report. We don't share even this when we create the issue, as it can be seen as breaking GitHub's Guidelines and Community rules (i.e. advertising).

@JamieSlome I looking at the proffered link without logging in, it looks like (from the context surrounding the blurred text) it is a case of a poor quality regex (i.e. CWE-1333 etc). I don't think we have any regex in the core of the library, or even in the examples we offer to users.
There are however regex in some of our internal tools we use when doing releases.
i.e.

BRAND_MODEL = re.compile(r"Brand: *(?P<brand>.+), *Model: *(?P<model>.+)")
ENUMS = re.compile(r"enum (\w+) {(.+?)};", re.DOTALL)
ENUM_ENTRY = re.compile(r"^\s+(\w+)", re.MULTILINE)
DECODED_PROTOCOLS = re.compile(r".*(?:results->decode_type *=.*?|"
r"typeguess\s*=\s*decode_type_t::)(\w+);")
AC_FN = re.compile(r"ir_(.+)\.h")
AC_MODEL_ENUM_RE = re.compile(r"(.+)_ac_remote_model_t")
IRSEND_FN_RE = re.compile(r"IRsend\.h")
ALL_FN = re.compile(r"ir_(.+)\.(h|cpp)")

And that tool (scrape_supported_devices.py) isn't even exposed to untrusted data.

As the "tools" directory is pretty much the only one with regex in/used by the code, it's of very low priority/importance in the scheme of things. Thus, I'm more than happy for your team to drop the details in a new (public) Issue on this repo.

Per your FAQ:

I'm a maintainer, but don't want to sign up. What should I do?

If you have been notified of a vulnerability disclosure against your repository, we will send a magic link to the elected e-mail address in your SECURITY.md. This magic link will allow you to view the private advisory, and validate/invalidate if necessary.
If you do not have a SECURITY.md in your repository, we will ping you the advisory information once you have added this.

Feel free to send the magic link to myself & @NiKiZe.
Per our code review policy, the Security.md will only be merged once it's been through our process. Thanks for your additional review, but I'll wait for @NiKiZe as he seems to have some thing to say on the issue.

@crankyoldgit
Copy link
Owner

And that tool (scrape_supported_devices.py) isn't even exposed to untrusted data.

Oh, that's incorrect now that I think about it. Someone could submit a PR with something evil in it. The worst that will happen is they get an RCE in a GitHub Actions container. Thankfully they (GitHub Actions) have no additional privileges above any other GitHub user w.r.t. this repo. So we are still fairly safe.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Sep 25, 2021

And that tool (scrape_supported_devices.py) isn't even exposed to untrusted data.

Oh, that's incorrect now that I think about it. Someone could submit a PR with something evil in it. The worst that will happen is they get an RCE in a GitHub Actions container. Thankfully they (GitHub Actions) have no additional privileges above any other GitHub user w.r.t. this repo. So we are still fairly safe.

I would be more worried about a code execution and running it on my local machine - but since we must review and approved any new contributors PRs this is probably low risk (even if we don't check for non visible chars)

@crankyoldgit
Copy link
Owner

@zidingz @JamieSlome @srikanthprathi In the interest of saving time, as it seems GitHub's "privacy" email system doesn't seem to work as advertised, please feel free to email me directly at david@xyzzy.com.au

@zidingz
Copy link
Author

zidingz commented Sep 28, 2021

Thank you for your patience and understanding!

An email should be with you in an hour.

@zidingz zidingz closed this as completed Sep 28, 2021
@crankyoldgit
Copy link
Owner

I'll keep any eye out for it.

@crankyoldgit
Copy link
Owner

@zidingz I haven't seen any email to the above address from you or your org yet.

crankyoldgit added a commit that referenced this issue Sep 29, 2021
@crankyoldgit
Copy link
Owner

FYI, the PR has been merged, the repo now as a Security.md that you can read/reference etc.

@crankyoldgit
Copy link
Owner

Confirming I got the email with the magic link.

crankyoldgit added a commit that referenced this issue Sep 29, 2021
Note: Tool is used internally by library developers to automatically generate documentation prior to a release and to warn when files are missing the required documentation.

* Update Regex used to eliminate potential denial of service.
  - Deemed a Low severity & impact threat.
  - Does NOT require or necessitate a new version/release of the library.
  - The tool is only used after review of code/PR etc which would already be very suspicious.
  - The tool is not something a user would typically use or run. (i.e. Internal use only)
  - Very unlikely to be exploited as the tool is not typically automatically run.
  - Expected worse case scenario would be the Continuous Integration tests failing due to a timeout, or a Developer losing some CPU time if they didn't notice a malicious PR/commit. (It would be very very obvious)
  - Remediation tested using supplied Proof of Concept code. i.e. Guaranteed Sub mSecond CPU use instead of exponential CPU use.
  - Addressed all other regex as a precaution as well via sensible range limitations.
* Documented the Regex used via `re.VERBOSE` and made easier to read/follow.
* Simplified some of the Regex.

Fixes #1616

Thanks to @srikanthprathi for reporting the issue & providing POC via the Huntr.Dev team.

Thanks to Huntr.Dev team for bring the issue to our attention. (Kudos to @zidingz, @JamieSlome)

Ref: https://huntr.dev/bounties/4da00a75-50dc-458b-acc6-cc216e1c854a/
crankyoldgit added a commit that referenced this issue Sep 29, 2021
Note: Tool is used internally by library developers to automatically generate documentation prior to a release and to warn when files are missing the required documentation.

* Update Regex used to eliminate potential denial of service.
  - Deemed a Low severity & impact threat.
  - Does NOT require or necessitate a new version/release of the library.
  - The tool is only used after review of code/PR etc which would already be very suspicious.
  - The tool is not something a user would typically use or run. (i.e. Internal use only)
  - Very unlikely to be exploited as the tool is not typically automatically run.
  - Expected worse case scenario would be the Continuous Integration tests failing due to a timeout, or a Developer losing some CPU time if they didn't notice a malicious PR/commit. (It would be very very obvious)
  - Remediation tested using supplied Proof of Concept code. i.e. Guaranteed Sub mSecond CPU use instead of exponential CPU use.
  - Addressed all other regex as a precaution as well via sensible range limitations.
* Documented the Regex used via `re.VERBOSE` and made easier to read/follow.
* Simplified some of the Regex.

Fixes #1616

Thanks to @srikanthprathi for reporting the issue & providing POC via the Huntr.Dev team.

Thanks to Huntr.Dev team for bring the issue to our attention. (Kudos to @zidingz, @JamieSlome)

Ref: https://huntr.dev/bounties/4da00a75-50dc-458b-acc6-cc216e1c854a/
crankyoldgit added a commit that referenced this issue Nov 19, 2021
_v2.8.0 (20211119)_

**[Bug Fixes]**
- Fix compilation issue when using old 8266 Arduino Frameworks. (#1639 #1640)
- Fix potential security issue with `scrape_supported_devices.py` (#1616 #1619)

**[Features]**
- SAMSUNG_AC
  - Change `clean` setting to a toggle. (#1676 #1677)
  - Highest fan speed is available without Powerful setting. (#1675 #1678)
  - Change `beep` setting to a toggle. (#1669 #1671)
  - Fix Beep for AR12TXEAAWKNEU (#1668 #1669)
  - Add support for Horizontal Swing & Econo (#1277 #1667)
  - Add support for On, Off, & Sleep Timers (#1277 #1662)
  - Fix power control. Clean-up code & bitmaps from Checksum changes. (#1277 #1648 #1650)
- HAIER_AC176/HAIER_AC_YRW02
  - Add support A/B unit setting (#1672)
  - Add support degree Fahrenheit (#1659)
  - Add support `Lock` function (#1652)
  - Implement horizontal swing feature (#1641)
  - Implement Quiet setting. (#1634 #1635)
- Basic support for Airton Protocol (#1670 #1681)
- HAIER_AC176: Add Turbo and Quiet settings (#1634)
- Gree: Add `SwingH` & `Econo` control. (#1587 #1653)
- MIRAGE
  - Add experimental detailed support. (#1573 #1615)
  - Experimental detailed support for KKG29A-C1 remote. (#1573 #1660)
- ELECTRA_AC: Add support for "IFeel" & Sensor settings. (#1644 #1645)
- Add Russian translation (#1649)
- Add Swedish translation (#1627)
- Reduce flash space used. (#1633)
- Strings finally in Flash! (#1493 #1614 #1623)
- Add support for Rhoss Idrowall MPCV 20-30-35-40 A/C protocol (#1630)
- Make `IRAc::opmodeToString()` output nicer for humans. (#1613)
- TCL112AC/TEKNOPOINT: Add support for `GZ055BE1` model (#1486 #1602)
- Support for Arris protocol. (#1598)
- SharpAc: Allow position control of SwingV (#1590 #1594)

**[Misc]**
- HAIER_AC176/HAIER_AC_YRW02
  - Replace some magic numbers with constants (#1679)
  - Small fix `Quiet` and `Turbo` test (#1674)
  - Fix `IRHaierAC176::getTemp()` return value description (#1663)
- Security Policy creation and changes. (#1616 #1617 #1618 #1621 #1680)
- IRrecvDumpV2/3: Update PlatformIO envs for missing languages (#1661)
- IRMQTTServer
  - Use the correct string for Fan mode in Home Assistant. (#1610 #1657)
  - Move a lot of the strings/text to flash. (#1638)
- Minor code style improvements. (#1656)
- Update Supported Devices
  - HAIER_AC176 (#1673)
  - LG A/C (#1651 #1655)
  - Symphony (#1603 #1605)
  - Epson (#1574 #1601)
  - GREE (#1587 #1588)
  - SharpAc (#1590 #1591)
- Add extra tests for LG2 protocol (#1654)
- Fix parameter expansion in several macros.
- Move some strings to `IRtext.cpp` & `locale/default.h` (#1637)
- RHOSS: Move include and defines to their correct places (#1636)
- Make makefile only build required files when running `run-%` target (#1632)
- Update Portuguese translation (#1628)
- Add possibility to run specific test case (#1625)
- Change `googletest` library ignore (#1626)
- Re-work "Fan Only" strings & matching. (#1610)
- Address `C0209` pylint warnings. (#1608)
crankyoldgit added a commit that referenced this issue Nov 19, 2021
## _v2.8.0 (20211119)_

**[Bug Fixes]**
- Fix compilation issue when using old 8266 Arduino Frameworks. (#1639 #1640)
- Fix potential security issue with `scrape_supported_devices.py` (#1616 #1619)

**[Features]**
- SAMSUNG_AC
  - Change `clean` setting to a toggle. (#1676 #1677)
  - Highest fan speed is available without Powerful setting. (#1675 #1678)
  - Change `beep` setting to a toggle. (#1669 #1671)
  - Fix Beep for AR12TXEAAWKNEU (#1668 #1669)
  - Add support for Horizontal Swing & Econo (#1277 #1667)
  - Add support for On, Off, & Sleep Timers (#1277 #1662)
  - Fix power control. Clean-up code & bitmaps from Checksum changes. (#1277 #1648 #1650)
- HAIER_AC176/HAIER_AC_YRW02
  - Add support A/B unit setting (#1672)
  - Add support degree Fahrenheit (#1659)
  - Add support `Lock` function (#1652)
  - Implement horizontal swing feature (#1641)
  - Implement Quiet setting. (#1634 #1635)
- Basic support for Airton Protocol (#1670 #1681)
- HAIER_AC176: Add Turbo and Quiet settings (#1634)
- Gree: Add `SwingH` & `Econo` control. (#1587 #1653)
- MIRAGE
  - Add experimental detailed support. (#1573 #1615)
  - Experimental detailed support for KKG29A-C1 remote. (#1573 #1660)
- ELECTRA_AC: Add support for "IFeel" & Sensor settings. (#1644 #1645)
- Add Russian translation (#1649)
- Add Swedish translation (#1627)
- Reduce flash space used. (#1633)
- Strings finally in Flash! (#1493 #1614 #1623)
- Add support for Rhoss Idrowall MPCV 20-30-35-40 A/C protocol (#1630)
- Make `IRAc::opmodeToString()` output nicer for humans. (#1613)
- TCL112AC/TEKNOPOINT: Add support for `GZ055BE1` model (#1486 #1602)
- Support for Arris protocol. (#1598)
- SharpAc: Allow position control of SwingV (#1590 #1594)

**[Misc]**
- HAIER_AC176/HAIER_AC_YRW02
  - Replace some magic numbers with constants (#1679)
  - Small fix `Quiet` and `Turbo` test (#1674)
  - Fix `IRHaierAC176::getTemp()` return value description (#1663)
- Security Policy creation and changes. (#1616 #1617 #1618 #1621 #1680)
- IRrecvDumpV2/3: Update PlatformIO envs for missing languages (#1661)
- IRMQTTServer
  - Use the correct string for Fan mode in Home Assistant. (#1610 #1657)
  - Move a lot of the strings/text to flash. (#1638)
- Minor code style improvements. (#1656)
- Update Supported Devices
  - HAIER_AC176 (#1673)
  - LG A/C (#1651 #1655)
  - Symphony (#1603 #1605)
  - Epson (#1574 #1601)
  - GREE (#1587 #1588)
  - SharpAc (#1590 #1591)
- Add extra tests for LG2 protocol (#1654)
- Fix parameter expansion in several macros.
- Move some strings to `IRtext.cpp` & `locale/default.h` (#1637)
- RHOSS: Move include and defines to their correct places (#1636)
- Make makefile only build required files when running `run-%` target (#1632)
- Update Portuguese translation (#1628)
- Add possibility to run specific test case (#1625)
- Change `googletest` library ignore (#1626)
- Re-work "Fan Only" strings & matching. (#1610)
- Address `C0209` pylint warnings. (#1608)
@crankyoldgit
Copy link
Owner

FYI, the changes mentioned above have now been included in the new v2.8.0 release of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants