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

Security issues: Notepad++ reads and writes past the end of a buffer bounds on opening a crafted file. #14073

Closed
Stefan1200-de opened this issue Aug 30, 2023 · 15 comments
Assignees
Labels

Comments

@Stefan1200-de
Copy link

Stefan1200-de commented Aug 30, 2023

Notepad++ reads and writes past the end of a buffer bounds on opening a crafted file.
This bugs still exists in v8.5.6

More details:
https://securitylab.github.com/advisories/GHSL-2023-092_Notepad__/

Reference numbers from GitHub Security Labs:
GHSL-2023-092, GHSL-2023-102, GHSL-2023-103, GHSL-2023-112

CVE numbers and score:
CVE-2023-40031 (7.8 High)
CVE-2023-40036 (5.5 Medium)
CVE-2023-40164 (5.5 Medium)
CVE-2023-40166 (5.5 Medium)

@Stefan1200-de Stefan1200-de changed the title Security issues: CVE-2023-40031, CVE-2023-40036, CVE-2023-40164, CVE-2023-40166 Security issues: Notepad++ reads and writes past the end of a buffer bounds on opening a crafted file. Aug 30, 2023
@dstrzelec
Copy link

Is there an estimate of when a release will be available that closes these vulns?

@slin-cls
Copy link

I see a possible cause of CVE-2023-40031 in the UniConversion.cxx, in the UTF8FromUTF16 function. The remaining size of the destination buffer is not checked in the loop.
As I see the code belongs to the Scintilla component. But as I'm not a contributor in this project, I don't know if the code is only the integration of the component or it is the Scintilla itself. If it comes from the Scintilla project, the bug should be fixed there first. Is anyone here who knows the origin of this code and where it should be fixed?

@domivogt
Copy link

The CVE is a ridiculous exaggeration of the situation. While it is technically a "buffer overflow" is really only an off-by-two bug with practically zero chance to allow for arbitrary code execution:

--

If the document has an odd length (malformed utf16), the CVE suggests that the incomplete last utf16 character at the end of the file is still converted to utf8.

But the length calculation is two bytes off:

newSize = len + len/2 + 1;

E.g. if "len" is 1, the formula results in 1 + 0 + 1 = 2 where it should be 4. The fix is very simple. Round up "len" before the calculation:

len = len + (len & 1); // If len is odd, add one to make it even.
newSize = len + len/2 + 1;

There is no way that this leads to arbitrary code execution.

--

There's probably also an off-by-one read bug in the same situation (the missing byte at the end of file must come from somewhere.)

@rdipardo
Copy link
Contributor

@slin-cls,

I don't know if the code is only the integration of the component or it is the Scintilla itself. If it comes from the Scintilla project, the bug should be fixed there first. Is anyone here who knows the origin of this code and where it should be fixed?

The only third-party code identified in the advisory comes from uchardet, which has already been patched in a newer version than what N++ currently uses. GHSL-2023-102/3 can be resolved with a simple dependency update.

Scintilla's core components are hosted on SourceForge and would never receive GitHub's attention.

@donho donho self-assigned this Aug 31, 2023
@donho
Copy link
Member

donho commented Aug 31, 2023

CVE-2023-40164 is merged: 5402622
GHSL-2023-112: A Change Request has been done, waiting reply.

Edit: GHSL-2023-112 is merged: 4b66d80
I'm gonna review 2 remain issues

@Nantris
Copy link

Nantris commented Sep 1, 2023

Since I haven't used it in a while I went to uninstall until there's an update. Since there's the existing concerns about security, when the unsigned installer asked for admin privileges I was in the mindset to virus-scan it. While it's marked clean by nearly every vendor, I found the uninstaller's behaviors/capabilities a bit concerning: https://www.virustotal.com/gui/file/7022af4ff6b2dcb3fd856d0d6f8b8b66faaa22068903e574954e2ed62272d667/relations

I'm no security expert so I don't know how common these are in generic uninstallers, but I don't know why it would need:

Open clipboard
Capture webcam image
Permissions Requested: SE_LOAD_DRIVER_PRIVILEGE (This in particular drew my attention)
Contacted domains: windowsupdatebg.s.llnwi.net

Can anyone shine some light?

@speneos
Copy link

speneos commented Sep 1, 2023

Thank you for the updates. @donho When is the expected release date for update please to resolve these CVEs? Thank you

@donho
Copy link
Member

donho commented Sep 1, 2023

@speneos
When it's ready.

@donho
Copy link
Member

donho commented Sep 2, 2023

CVE-2023-40164 merged: 5402622
GHSL-2023-112 merged: 4b66d80
GHSL-2023-102 merged: ea06324
GHSL-2023-103 merged: 8c561ba

Thank @JarLob for his contribution!

@aledeniz
Copy link

aledeniz commented Sep 5, 2023

Since I haven't used it in a while I went to uninstall until there's an update. Since there's the existing concerns about security, when the unsigned installer asked for admin privileges I was in the mindset to virus-scan it. While it's marked clean by nearly every vendor, I found the uninstaller's behaviors/capabilities a bit concerning: https://www.virustotal.com/gui/file/7022af4ff6b2dcb3fd856d0d6f8b8b66faaa22068903e574954e2ed62272d667/relations

This is unrelated, you should have raised a separate issue.

@realityexists
Copy link

realityexists commented Sep 20, 2023

The timeline posted in the linked advisory paints a very bleak picture of this project's maintainer's attitude to security. 4 months and 4 releases with fixes such as Minor style bug, but without the fix for a critical vulnerability (arbitrary code execution), no replies to the security researcher repeatedly trying to get this fixed, and the fix was only finally released weeks after public disclosure? Anyone ever opening files they don't completely trust must seriously consider using a different editor.

@vablings
Copy link

vablings commented Oct 2, 2023

The timeline posted in the linked advisory paints a very bleak picture of this project's maintainer's attitude to security. 4 months and 4 releases with fixes such as Minor style bug, but without the fix for a critical vulnerability (arbitrary code execution), no replies to the security researcher repeatedly trying to get this fixed, and the fix was only finally released weeks after public disclosure? Anyone ever opening files they don't completely trust must seriously consider using a different editor.

#14073 (comment)

Please learn to read and count

@eabase
Copy link

eabase commented Nov 18, 2023

Since I haven't used it in a while I went to uninstall until there's an update. Since there's the existing concerns about security, when the unsigned installer asked for admin privileges I was in the mindset to virus-scan it. While it's marked clean by nearly every vendor, I found the uninstaller's behaviors/capabilities a bit concerning: https://www.virustotal.com/gui/file/7022af4ff6b2dcb3fd856d0d6f8b8b66faaa22068903e574954e2ed62272d667/relations

I'm no security expert so I don't know how common these are in generic uninstallers, but I don't know why it would need:

Open clipboard
Capture webcam image
Permissions Requested: SE_LOAD_DRIVER_PRIVILEGE (This in particular drew my attention)
Contacted domains: windowsupdatebg.s.llnwi.net

Can anyone shine some light?

This is incredibly disturbing. The uninstaller is calling the content delivery network and collecting significant personal and os info that should not be exfiltrated from your computer.

Please file a separate issue.

@Nantris
Copy link

Nantris commented Nov 18, 2023

@eabase see #14368. I won't have time to follow up on this. I removed Notepad++ entirely from my devices. I'm not sure how much attention an issue can get when there are over 2k open.

@xomx
Copy link
Contributor

xomx commented Nov 21, 2023

when the unsigned installer asked for admin privileges I was in the mindset to virus-scan it

From the v8.5.7+ the N++ uninstaller is signed ( 4476432 ).

And the uninstaller needs the admin privileges to uninstall software from a protected location (...\Program Files\...).

The uninstaller is calling the content delivery network and collecting significant personal and os info that should not be exfiltrated from your computer.

Barking up the wrong tree. Try to persuade MS & all the others to do not collect such telemetry data...
N++ is opensource (including its NSIS un/installer script), so anybody can check and build their own N++ un/installer for testing... (except the final code signing, of course).

Permissions Requested: SE_LOAD_DRIVER_PRIVILEGE (This in particular drew my attention)

While I can confirm that the VirusTotal is showing that suspicious behavior for the N++ uninstaller, I did not find anything like that myself: #14368 (comment)

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

No branches or pull requests