Warn if Windows compatibility mode flags are detected on startup

[smallketchup82]

Closes #22092

Description

This PR adds a new component, CompatibilityModeChecker.cs to osu.Desktop/Windows, which does as the name implies.

Windows writes to HKCU\Software\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\Layers to track compatibility flags for applications. The key names in the registry folder coincide with the path to the executables. Following this logic, we can check each path in the registry folder to see if they contain osu!.exe which would likely mean a path to the lazer installation. If this is detected, the component gives the user a notification to warn them about using compatibility mode.

Lazer in general already refuses to run under compatibility mode (since it requires windows 8.1 or later) but the same can't be said for other values, such as fullscreen optimizations, dpi optimizations, etc. By using the registry, we can make sure that virtually every value that would be in the properties -> compatibility tab is turned off. Windows automatically deletes the keys if there are no compatibility options set, so we really only have to check that a key pointing to the lazer executable exists, and if so, warn the user.

Concerns

I do have a bit of concerns with this implementation. I could have addressed the concerns below before making the PR, but for the sake of simplicity, I decided to leave them out pending discussion.

1. Checking for osu!.exe seems a bit hacky, it would probably be better to try and find matches to the runtime executable. Although I'm not sure if this would work properly since it looks like there's two executables, an entry osu!.exe, and another osu!.exe in the squirrel installation folder
   • This could also possibly trigger if there are compatibility options set for stable
   • It could be wise to wait until peppy finishes his squirrel update, then come back to this
2. The notification strings should probably be made translatable, both in this component and in ElevatedPrivilegesChecker.cs
3. There also exists HKLM\Software\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\Layers which similarly holds compatibility flag information. In my experience, this is rarely used and is only written to if the user presses "change settings for all users" when making the compatibility mode changes. Although I personally don't think that it's worth it since a user isn't likely to do this, we could also track this to be safe (grab the array of names from HKLM, combine it with the array from HKCU, then look through the final concatenated array instead)
4. Instead of simply notifying the user, we could outright disallow using any compatibility mode flags as peppy discussed in the issue thread. I personally don't think we need to go to that extreme, but it is an option
5. We can also turn off the flags for the user by deleting the registry key, but I don't believe we should do this since it'd be best to avoid touching the registry unless we need to

If the concerns above aren't pressing, then this implementation should be capable of easily checking to see if there are any compatibility flags set for the application

[SupDos]

but the same can't be said for other values, such as fullscreen optimizations

Hope I'm not being too annoying here, but is there any reason why you're also including the "Disable full-screen optimizations" checkbox to show this warning? This is what tells Windows (or well, suggests...) to allow Exclusive Fullscreen instead of forcing the game to run in borderless, and isn't something that would affect the game negatively.

[smallketchup82]

but the same can't be said for other values, such as fullscreen optimizations

Hope I'm not being too annoying here, but is there any reason why you're also including the "Disable full-screen optimizations" checkbox to show this warning? This is what tells Windows (or well, suggests...) to allow Exclusive Fullscreen instead of forcing the game to run in borderless, and isn't something that would affect the game negatively.

That checkbox is to disable these optimizations. It is a setting meant for compatibility (such as if a program is having issues). Lazer will be able to use borderless and exclusive normally since fullscreen optimizations aren't disabled by default, and lazer never turns them off (afaik). With the setting checked on, windows disables the optimizations that it would otherwise runs for lazer (such as optimizations on overlays). Additionally, fullscreen optimizations on windows were created especially for games. Giving the user the ability to disable these adds another vector for things to go wrong. Best not to risk it at all.

This article should explain full screen optimizations in depth: https://devblogs.microsoft.com/directx/demystifying-full-screen-optimizations/

If there's evidence that lazer is inadvertently flipping this setting on, then I could program in an exclusion for that specific case. But at present it doesn't seem like lazer needs to disable full-screen optimizations, since there's really no downside to having it on, but there are downsides to having it off.

[bdach]

So I tested this diff and... I think trying to find out if compatibility mode is enabled it makes windows defender angry...

What I've done is I set up a mock release with osu-deploy on this branch and installed it. The path to the executable needs fixing (right now it's pointing to the dll and not the exe), but after fixing that... I've had windows defender quarantine the game executable twice due to Trojan:Script/Wacatac.B!ml. Which means that either my machine and build toolchain is compromised and I need to start tearing everything to the ground right now, or (more likely, looking at google results) this is a bullcrap "machine" "learning" "detection" scaremonger. That also means that we probably can't ship it either because users will get scared out of their minds. I don't think this is an important enough change to warrant the headache to clear the false positive later.

Before you ask, this doesn't happen if I do the same local release procedure on master, so it looks pretty evident that it's the registry read that defender isn't liking.

[smoogipoo]

It should not get quarantined after the app is signed (part of our deploy process), but regardless that is pretty grim especially if it affects development.

I agree this is not an important enough issue to be addressed, though.

[smallketchup82]

So I tested this diff and... I think trying to find out if compatibility mode is enabled it makes windows defender angry...

What I've done is I set up a mock release with osu-deploy on this branch and installed it. The path to the executable needs fixing (right now it's pointing to the dll and not the exe), but after fixing that... I've had windows defender quarantine the game executable twice due to Trojan:Script/Wacatac.B!ml. Which means that either my machine and build toolchain is compromised and I need to start tearing everything to the ground right now, or (more likely, looking at google results) this is a bullcrap "machine" "learning" "detection" scaremonger. That also means that we probably can't ship it either because users will get scared out of their minds. I don't think this is an important enough change to warrant the headache to clear the false positive later.

Before you ask, this doesn't happen if I do the same local release procedure on master, so it looks pretty evident that it's the registry read that defender isn't liking.

I'll look into this myself and see if I can resolve it. If not I'll probably close this PR myself since this issue definitely doesn't warrent dealing with defender. Could you send the code that points to the exe instead of the dll so that I can test it on my machine and try to find a workaround?

[bdach]

The relevant change is

diff --git a/osu.Desktop/Windows/CompatibilityModeChecker.cs b/osu.Desktop/Windows/CompatibilityModeChecker.cs
index 31f32441d7..69765ed319 100644
--- a/osu.Desktop/Windows/CompatibilityModeChecker.cs
+++ b/osu.Desktop/Windows/CompatibilityModeChecker.cs
@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Diagnostics;
 using System.Linq;
 using System.Reflection;
 using System.Runtime.Versioning;
@@ -35,9 +36,7 @@ public static bool CheckCompatibilityMode()
         {
             using var layers = Registry.CurrentUser.OpenSubKey(@"Software\Microsoft\Windows NT\CurrentVersion\AppCompatFlags\Layers");
 
-            string exePath = Assembly.GetExecutingAssembly().Location;
-
-            return layers != null && layers.GetValueNames().Any(name => name.Equals(exePath, StringComparison.OrdinalIgnoreCase));
+            return layers != null && layers.GetValueNames().Any(name => name.Equals(Environment.ProcessPath, StringComparison.OrdinalIgnoreCase));
         }
 
         private partial class CompatibilityModeNotification : SimpleNotification

I believe.

[smallketchup82]

Reading this, the issue might be with the exe location code rather than the registry? I'll test this locally. Although I don't see anything about this process that would be similar at all to malware. We aren't modifying the registry, and doing a read call to HKCR as a non-admin process is about as innocent as it gets.

[smallketchup82]

@bdach Quick question, if you still have the exe that's causing issues, could you send it to me (either via discord or a link)? I'd like to do malware analysis on it to figure out what fingerprints the ml is detecting in particular.

Apart from a conveniently timed notification from windows defender to do a security scan while starting lazer, I can't seem to reproduce this. Though I haven't yet tried it with osu-deploy (the lack of docs makes it a pain to figure out) though I'm working on trying to reproduce with osu-deploy.

[bdach]

Though I haven't yet tried it with osu-deploy (the lack of docs makes it a pain to figure out) though I'm working on trying to reproduce with osu-deploy.

Please do try that first. There should be no docs required. You just run the thing on windows. So long as osu and osu-deploy are in one common dir it should just work ™️ .

Although if you're going to try that then probably it's best that you apply

diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs
index 81e3d8bed8..edd8246d73 100644
--- a/osu.Game/OsuGameBase.cs
+++ b/osu.Game/OsuGameBase.cs
@@ -98,7 +98,7 @@ public partial class OsuGameBase : Framework.Game, ICanAcceptFiles, IBeatSyncPro
         /// </summary>
         private const double global_track_volume_adjust = 0.8;
 
-        public virtual bool UseDevelopmentServer => DebugUtils.IsDebugBuild;
+        public virtual bool UseDevelopmentServer => true;
 
         public virtual EndpointConfiguration CreateEndpoints() =>
             UseDevelopmentServer ? new DevelopmentEndpointConfiguration() : new ProductionEndpointConfiguration();
@@ -110,7 +110,7 @@ public partial class OsuGameBase : Framework.Game, ICanAcceptFiles, IBeatSyncPro
         /// </summary>
         public string VersionHash { get; private set; }
 
-        public bool IsDeployedBuild => AssemblyVersion.Major > 0;
+        public bool IsDeployedBuild => false;
 
         internal const string BUILD_SUFFIX = "lazer";
 

so that you can use dev credentials and dev website instance for safer testing.

[smallketchup82]

Got it, will definitely try

[smallketchup82]

@bdach Using the exact patches and osu-deploy guide that you sent, simulating a mock release using osu-deploy, I cannot reproduce the windows defender issue. I've verified that I'm running the correct version — Enabling compatibility mode shows the new messagebox. I've also done this on a fresh windows VM to rule out the possibility of defender exclusions resolving it

A defender scan of the entire AppData/Local/osulazer directory shows nothing wrong

I've also uploaded my output exe to virustotal for a 2nd opinion, it has come back entirely positive.
https://www.virustotal.com/gui/file-analysis/ZWIzODI5ZWY3M2FkMWMwOGEyZGJhMTJjMGExNDZkZjA6MTcxMTEyNjc4OA==

[bdach]

I cannot reproduce it either now. But it did happen, I am not crazy.

It might be because windows nudged their "ai" "detection" bullshit.

I don't even know where to begin with this now. But I'm still reluctant to merge this after that experience. I'm not sure it's even providing that much benefit to anyone.

[smallketchup82]

I cannot reproduce it either now. But it did happen, I am not crazy.

It was probably resolved by a windows defender definition update in that case. Though you should still try to unquarantine the offending exe and scan again to make sure.

But I'm still reluctant to merge this after that experience.

I don't think that the scare warrants not merging, ever. Sure this isn't that important of a change, but it does help defend against end users messing with their installation then coming here complaining. It also seemed to be an edge case experienced by only you, since my builds yesterday (though not using osu-deploy) also didn't return anything after checking for exclusions, doing a local defender scan, doing a virustotal scan, and doing the same process but in windows sandbox.

I think a better way to go about it is blocking the pr for 2 weeks up to a month then revisiting after we can be certain that a number of users have updated their definitions (which should be happening daily). Then we can also have someone like peppy, frenzi, or smoogi also try building to see what result they get.

[peppy]

Shall we apply the filename fix and get this in?

As mentioned, windows defender pings aren't worth worrying about – that's for microsoft to fix (unless we're doing something really weird).

[bdach]

If you're not worried then I guess 🤷

[smallketchup82]

I'll commit the filename patch shortly

But I would like to ask, wouldn't it be wise to log the value of the registry key after having checked for it? I feel as though if this is blocking people from launching the game, it would be wise to include what is blocking the game from launching in the debug logs. If someone comes here complaining about it, we can easily tell them what to turn off by looking at their debug logs instead of having to teach them to navigate the registry editor and to find the offending option.

Should I make that change or are we fine?

[bdach]

I would like to think the only option that would block the game from launching is the "pretend to be windows 8 or earlier" option. But I guess no harm in enabling.

[smallketchup82]

@bdach Along with some of my final touches, are we sure that we don't want to make the notification string translatable (one of my original concerns listed)? I believe it would be helpful, but at the same time, given that the string for the windows defender check also isn't translatable, I think it could be better to add that functionality for both in a later PR.

[bdach]

notification string translatable

not sure why that requires pinging me specifically but whatever

the in-game text you can make translatable i guess. you're gonna have more trouble with the text that shows when the game refuses to launch because at that point none of the framework localisation machinery is active yet. so i'd suggest not bothering with that part specifically

[smallketchup82]

Should be all good for merge

[Susko3]

Please test your changes ¯\_(ツ)_/¯