Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1503 »

Doc Hawkins: Jun 15, 2010; Dashing? But I'm not even moving!

ultrafilter posted:

Someone who has access to edit that file probably has more direct options to do whatever they're trying to do.

yeah, but you could make it look like cyberpunk's fault, it's the perfect crime

(i kid)

# ? Dec 12, 2020 00:27

Adbot: ADBOT LOVES YOU

# ? May 14, 2024 11:58

Volte: Oct 4, 2004; woosh woosh

QuarkJets posted:

This probably isn't a threat to most business networks, but is a threat to tons of individuals. For instance, launch a persistent keylogger prior to launching the game. Or launch something that requests privilege escalation because most users just want to play their game and will click "OK" immediately.

This isn't overriding the location pointed to by Steam, it's taking advantage of the fact that anyone can edit the JSON that the Cyberpunk launcher reads from when the user presses a button.

(and this isn't unique to Cyberpunk; many game launchers have this kind of vulnerability)

If that's all it is, then that seemingly barely qualifies as a vulnerability. I mean instead of editing the JSON file it could just replace the exe file wholesale with a compromised one if it has permission to do that, which could apply to any game, even ones without launchers. Hell, if it has permission to do that, it could have just installed the keylogger as a service instead.

# ? Dec 12, 2020 00:42

biznatchio: Mar 31, 2001; Buglord

Shocking new exploit discovered! If you can edit files on the user's drive, you can replace the Cyberpunk launcher EXE with any arbitrary executable code of your choosing! Come on CDPR, this is amateur hour!

# ? Dec 12, 2020 01:39

Dylan16807: May 12, 2010

It's not about getting access, it's that you can bypass any signature verification or whitelisting.

Not a big deal with a game, but it can be a real problem with programs that are installed on locked-down computers.

# ? Dec 12, 2020 01:53

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

Are you envisioning an allowlisting system where if a trusted executable is allowlisted, it's allowed to invoke anything at all without any checks?

# ? Dec 12, 2020 02:00

xtal: Jan 9, 2011; by Fluffdaddy

Jabor posted:

Are you envisioning an allowlisting system where if a trusted executable is allowlisted, it's allowed to invoke anything at all without any checks?

Envisioning as in "looking right at," yes

# ? Dec 12, 2020 02:02

Volmarias: Dec 31, 2002; EMAIL... THE INTERNET... SEARCH ENGINES...

Dylan16807 posted:

It's not about getting access, it's that you can bypass any signature verification or whitelisting.

Not a big deal with a game, but it can be a real problem with programs that are installed on locked-down computers.

If they're that locked down how are you installing this anyway

# ? Dec 12, 2020 02:02

Dylan16807: May 12, 2010

Volmarias posted:

If they're that locked down how are you installing this anyway

Yes, it's not a big deal for a game, because that's probably not going to be installed and whitelisted by the sysadmin. But it's still fun to poke at a flaw like that.

# ? Dec 12, 2020 02:53

QuarkJets: Sep 8, 2008

Volte posted:

If that's all it is, then that seemingly barely qualifies as a vulnerability. I mean instead of editing the JSON file it could just replace the exe file wholesale with a compromised one if it has permission to do that, which could apply to any game, even ones without launchers. Hell, if it has permission to do that, it could have just installed the keylogger as a service instead.

The JSON string executes under the signed launcher, giving it a veneer of legitimacy that an arbitrary exe wouldn't have.

# ? Dec 12, 2020 07:01

Volmarias: Dec 31, 2002; EMAIL... THE INTERNET... SEARCH ENGINES...

QuarkJets posted:

The JSON string executes under the signed launcher, giving it a veneer of legitimacy that an arbitrary exe wouldn't have.

I think we get that it's a vulnerability and why, we're just questioning the narrow circumstances that would make this specific action worthwhile. You have to have a machine where cyberpunk is installed, and where unsigned applications are not allowed to run, but where you have gained write access, but you cannot write to something more useful, but you specifically want to pop this machine, but you somehow do not have any better exploits to chain.

# ? Dec 12, 2020 07:13

taqueso: Mar 8, 2004

the important thing is that their signing key be revoked immediately

# ? Dec 12, 2020 07:26

QuarkJets: Sep 8, 2008

I was under the impression that the file in question was left as user-writable, with no escalation required. So you edit that file, ask for elevation for some malicious code, and then the user (if they're able) provides elevation because it appears to be Cyberpunk asking for elevation.

# ? Dec 12, 2020 07:30

Dylan16807: May 12, 2010

Volmarias posted:

I think we get that it's a vulnerability and why, we're just questioning the narrow circumstances that would make this specific action worthwhile. You have to have a machine where cyberpunk is installed, and where unsigned applications are not allowed to run, but where you have gained write access, but you cannot write to something more useful, but you specifically want to pop this machine, but you somehow do not have any better exploits to chain.

There's also "the user is trying to run code and they're not supposed to be allowed to run non-whitelisted code".

Possibly of their own volition, or possibly because you tricked them.

# ? Dec 12, 2020 07:35

Bonfire Lit: Jul 9, 2008; If you're one of the sinners who caused this please unfriend me now.

xtal posted:

Envisioning as in "looking right at," yes

Why is that not a problem with signed executable explorer.exe in your system then?

# ? Dec 12, 2020 09:19

Sistergodiva: Jan 3, 2006; I'm like you,
I have no shame.

Dylan16807 posted:

Yes, it's not a big deal for a game, because that's probably not going to be installed and whitelisted by the sysadmin. But it's still fun to poke at a flaw like that.

A internet Cafe or LAN place locked down to only steam maybe?

# ? Dec 12, 2020 11:43

1337JiveTurkey: Feb 17, 2005

Honestly it's less of a horror than everyone standardizing on the same minigame to protect their data and money in the future.

# ? Dec 12, 2020 14:31

Volte: Oct 4, 2004; woosh woosh

Does Windows 10 really use a hierarchical code signing model where any process that gets spawned by a signed binary is considered signed by the certificate that signed the parent executable? And this is the default? How do Steam or any other launcher get around it? What should the CP2077 launcher have done differently? What's the use case of "all binaries must be signed, unless its parent process is signed, then whatever, who cares"? I'm trying to Google for examples of this vulnerability in game launchers or anywhere and I'm not turning up anything or even sure what to search for. The only system policy I can even find that would restrict a system to signed binaries is AppLocker, and I can find plenty of examples of child processes failing to launch because of not being in the AppLocker whitelist (and even if they didn't, that feels like it would be a flaw in AppLocker).

I do believe that smarter people than me think this is a vulnerability but so far nobody has actually demonstrated why. Has this been shown to be able to circumvent system policy in a proof of concept? The original tweet of pushing a button and opening calc.exe says about as much to me as adding calc.exe as a custom game to Steam and then launching it from there. Steam just has a built-in interface to modify its JSON-configuration equivalent.

# ? Dec 12, 2020 15:16

Achmed Jones: Oct 16, 2004

If the config file is world-writable, it allows users to target one another pretty easily. Quark jets said:

quote:

it's taking advantage of the fact that anyone can edit the JSON that the Cyberpunk launcher reads from when the user presses a button.

If it's accurate that the config file is world-writeable it's a pretty clear spot to move laterally or escalate. If it's not world writeable, the signing stuff isn't great but isn't the same level of bad.

Idk what windows does these days, but osx asks permission to run unsigned binaries in finder. If someone is used to signed code being an actually enforced(-ish) thing, evading signing looks worse than it does to people who only equate signed binaries with strict enterprise IT policies

# ? Dec 12, 2020 17:33

Volte: Oct 4, 2004; woosh woosh

A quick Google confirms that it's in the game directory, i.e. the same directory as the launcher executable itself. I'm not sure about the permissions, but unless it's explicitly made world-writable, it shouldn't create any new vulnerabilities beyond what having the game directory itself be world-writable would.

# ? Dec 12, 2020 17:52

Achmed Jones: Oct 16, 2004

~~Sorry I don't follow - does that have a particular implication with respect to it being world-writeable?~~

E: disregard you ninja'd in a clarifying bit. Thanks! Sounds like it's prob not works writeable

Achmed Jones fucked around with this message at 17:56 on Dec 12, 2020

# ? Dec 12, 2020 17:54

Plorkyeran: Mar 22, 2007; To Escape The Shackles Of The Old Forums, We Must Reject The Tribal Negativity He Endorsed

Jabor posted:

Are you envisioning an allowlisting system where if a trusted executable is allowlisted, it's allowed to invoke anything at all without any checks?

I would like to imagine that things have gotten better since then, but launching UT via Word to bypass the checks was a thing we did when I was in high school...

# ? Dec 12, 2020 21:10

QuarkJets: Sep 8, 2008

Plorkyeran posted:

I would like to imagine that things have gotten better since then

Quite an imagination you have there!

# ? Dec 13, 2020 00:03

M31: Jun 12, 2012

I think the idea is that you distribute the launcher along with your malicious code and just ignore that there was originally a game attached.

# ? Dec 13, 2020 00:59

Foxfire_: Nov 8, 2010

Volte posted:

Does Windows 10 really use a hierarchical code signing model where any process that gets spawned by a signed binary is considered signed by the certificate that signed the parent executable?

It does not.
Leaving Cyberpunk aside, did you know that cmd.exe is installed on your computer right now, is signed by Microsoft, and will run whatever command you pass to it! :kingsley:

HACKERS! posted:

cmd.exe /C calc

# ? Dec 13, 2020 01:24

Tei: Feb 19, 2011

the json could have a hash made from it to make sure is not exploited against the user

this also show that digitally signed binaries are a defense in deep - just create a easy to jump obstacle

# ? Dec 16, 2020 08:54

Captain Muffin: Apr 25, 2007; Women like you are the reason this chickens late in the first place.

In a trying not to start anything big before Christmas, I've picked up my ex managers ASP.NET Framework project, to try and sort out some logging for someone else.

Just need to setup NLog, can't quite see where the dependency injection is being set up.

Lets have a look at one of the Controllers:

code:

public class HomeController : Controller
{
	private readonly IFleetService fleetService;
	private readonly ISoftwareUpdateService softwareService;

	public HomeController(IFleetService fleetService, ISoftwareUpdateService softwareService)
	{
		this.fleetService = fleetService;
		this.softwareService = softwareService;
	}

	/* Poor man's DI, change to use Ninject*/
	public HomeController() 
		: this(new FleetService(new FleetRepository(), new ScanFleet(new VehicleBuilder(), new ReadVehicleComponents())), new SoftwareUpdateService(new SoftwareUpdateRepository()))
	{
	}

See, all you need to do is make everything have a parameter-less constructor, and that can instantiate everything and call another instructor via chaining! Bonus points for 'poor mans DI' comment. He has done this in every controller. The code has been like this for two years.

I regret offering to look at this.

# ? Dec 22, 2020 12:10

Hammerite: Mar 9, 2007; And you don't remember what I said here, either, but it was pompous and stupid.; Jade Ear Joe

Captain Muffin posted:

In a trying not to start anything big before Christmas, I've picked up my ex managers ASP.NET Framework project, to try and sort out some logging for someone else.

Just need to setup NLog, can't quite see where the dependency injection is being set up.

Lets have a look at one of the Controllers:
code:
public class HomeController : Controller
{
	private readonly IFleetService fleetService;
	private readonly ISoftwareUpdateService softwareService;

	public HomeController(IFleetService fleetService, ISoftwareUpdateService softwareService)
	{
		this.fleetService = fleetService;
		this.softwareService = softwareService;
	}

	/* Poor man's DI, change to use Ninject*/
	public HomeController() 
		: this(new FleetService(new FleetRepository(), new ScanFleet(new VehicleBuilder(), new ReadVehicleComponents())), new SoftwareUpdateService(new SoftwareUpdateRepository()))
	{
	}
See, all you need to do is make everything have a parameter-less constructor, and that can instantiate everything and call another instructor via chaining! Bonus points for 'poor mans DI' comment. He has done this in every controller. The code has been like this for two years.

I regret offering to look at this.

I can't really comment on the practice, since my knowledge of DI is pretty basic, but the thing that leaps out to me is that the implementation doesn't appear to be self-consistent. If everything has a parameterless constructor that constructs the default implementations of its dependencies, then why does the parameterless constructor for HomeController not in turn call the parameterless constructors of its dependencies? Why does it go to the trouble of constructing the dependencies of its dependencies?

i.e. the code you quoted says this:

code:

public A(): this(new P(new W(), new X()), new Q(new Y(), new Z()))
{}

to be consistent shouldn't it instead say this:

code:

public A(): this(new P(), new Q())
{}

where the parameterless constructors of P and Q are defined thus:

code:

public P(): this(new W(), new X())
{}

code:

public Q(): this(new Y(), new Z())
{}

It doesn't appear to obey its own principles (such as they are).

side note: lol at being horrified that code has been in a state of which you don't approve for two whole years. I've found (and posted in this thread) production code that was outright wrong for about a decade. I'm talking about code that just plain does the wrong thing, not just bad style.

# ? Dec 22, 2020 18:20

Hammerite: Mar 9, 2007; And you don't remember what I said here, either, but it was pompous and stupid.; Jade Ear Joe

This is still my favourite thing I've ever posted in this thread, I think

Hammerite posted:

Found today in maths code that's been around and in use for years.
code:
if (dt<dc)
{
	ar=ac; ac=at;
	dr=dc; dc=dt;
}
else
	al=at; dl=dt;

# ? Dec 22, 2020 18:39

Macichne Leainig: Jul 26, 2012; by VG

Took me probably far too long to understand what the problem was. Perhaps I am the coding horror.

# ? Dec 22, 2020 18:45

raminasi: Jan 25, 2005; a last drink with no ice

Captain Muffin posted:

In a trying not to start anything big before Christmas, I've picked up my ex managers ASP.NET Framework project, to try and sort out some logging for someone else.

Just need to setup NLog, can't quite see where the dependency injection is being set up.

Lets have a look at one of the Controllers:
code:
public class HomeController : Controller
{
	private readonly IFleetService fleetService;
	private readonly ISoftwareUpdateService softwareService;

	public HomeController(IFleetService fleetService, ISoftwareUpdateService softwareService)
	{
		this.fleetService = fleetService;
		this.softwareService = softwareService;
	}

	/* Poor man's DI, change to use Ninject*/
	public HomeController() 
		: this(new FleetService(new FleetRepository(), new ScanFleet(new VehicleBuilder(), new ReadVehicleComponents())), new SoftwareUpdateService(new SoftwareUpdateRepository()))
	{
	}
See, all you need to do is make everything have a parameter-less constructor, and that can instantiate everything and call another instructor via chaining! Bonus points for 'poor mans DI' comment. He has done this in every controller. The code has been like this for two years.

I regret offering to look at this.

Poor Man's Dependency Injection is a named pattern that's been around for a while, and it has at least one respected proponent in these very forums. I personally don't like it, but it has a case. However, your old manager did it wrong, and should have done it like this:

Hammerite posted:

I can't really comment on the practice, since my knowledge of DI is pretty basic, but the thing that leaps out to me is that the implementation doesn't appear to be self-consistent. If everything has a parameterless constructor that constructs the default implementations of its dependencies, then why does the parameterless constructor for HomeController not in turn call the parameterless constructors of its dependencies? Why does it go to the trouble of constructing the dependencies of its dependencies?

# ? Dec 22, 2020 20:27

New Yorp New Yorp: Jul 18, 2003; Only in Kenya.; Pillbug

raminasi posted:

Poor Man's Dependency Injection is a named pattern that's been around for a while, and it has at least one respected proponent in these very forums. I personally don't like it, but it has a case. However, your old manager did it wrong, and should have done it like this:

Seconding this. The practice is fine but the implementation here is dead wrong.

I actually recommend following this pattern (correctly) to people who are experienced developers but are new to testing practices because I find that it helps it click for them faster if they're constructing dependencies through some mechanism that's less opaque than a DI container.

# ? Dec 23, 2020 01:53

mod saas: May 4, 2004; Grimey Drawer

Hammerite posted:

This is still my favourite thing I've ever posted in this thread, I think

Protocol7 posted:

Took me probably far too long to understand what the problem was. Perhaps I am the coding horror.

Does it always invoke dl=dt; ?

# ? Dec 23, 2020 02:19

necrotic: Aug 2, 2005; I owe my brother big time for this!

Yes.

# ? Dec 23, 2020 02:37

Captain Muffin: Apr 25, 2007; Women like you are the reason this chickens late in the first place.

New Yorp New Yorp posted:

Seconding this. The practice is fine but the implementation here is dead wrong.

I actually recommend following this pattern (correctly) to people who are experienced developers but are new to testing practices because I find that it helps it click for them faster if they're constructing dependencies through some mechanism that's less opaque than a DI container.

Yeah if you just wanna prototype something quick its acceptable, and DI is super confusing if you don't know how its setup. This was all implemented by my manager who used to randomly quiz you about the SOLID C# principles every five minutes.

The best bit is he has one controller called home, so if you go to http://127.0.0.1:8080/home this gets called:

code:

public HomeController() : this(new FleetService(new FleetRepository(), new ScanFleet(new VehicleBuilder(), new ReadVehicleComponents())), new SoftwareUpdateService(new SoftwareUpdateRepository()))

and another controller called softwareupdate, so visiting: http://127.0.0.1:8080/softwareupdate uses this object

code:

public SoftwareUpdateController() : this(new SoftwareUpdateService(new SoftwareUpdateRepository()))

Both constructors create their own instance of

code:

new SoftwareUpdateService(new SoftwareUpdateRepository())

SoftwareUpdateRepository is a thing that acts like a database of what software updates are available for clients to download on the server. So yes, depending on what web page you are accessing, the home page can have a completely different set of updates available than the softwareupdate page. Mmmmmmmm. And its installed on a customer site and robots use this to get software updates :filez:

# ? Dec 23, 2020 10:22

Volguus: Mar 3, 2009

Hey, it could have been worse, they could have been a singleton.

# ? Dec 23, 2020 14:58

LOOK I AM A TURTLE: May 22, 2003; "I'm actually a tortoise."; Grimey Drawer

If that manually implemented DI is the worst thing in the project I think you're doing okay. Even the unnecessary chained constructor calls are kind of interesting in that they reveal the dependency hierarchy in a very explicit manner. Definitely wouldn't do it that way though. I do similar things in hybrid unit/integration tests sometimes, but I put the instantiation in the test code instead.

Captain Muffin posted:

Both constructors create their own instance of
code:
new SoftwareUpdateService(new SoftwareUpdateRepository())
SoftwareUpdateRepository is a thing that acts like a database of what software updates are available for clients to download on the server. So yes, depending on what web page you are accessing, the home page can have a completely different set of updates available than the softwareupdate page. Mmmmmmmm. And its installed on a customer site and robots use this to get software updates

Ah, there's the real problem. The repository probably shouldn't store the data in memory, and if it does it should be a singleton, or use a singleton memory cache of some sort. You can combine singletons with poor man's DI pretty smoothly by having a static instance variable on the class and no public constructors.

It sounds like you already know all of this, but I think it's interesting to get to the heart of the problem. Manual DI could be said to be the cause of the bug because the dev forgot about the shared dependency with local data, but you can easily get the same bug (and I've done this personally multiple times) by setting it up with the wrong lifetime in the DI framework. So the problem is more semantic than structural.

# ? Dec 25, 2020 11:04

NihilCredo: Jun 6, 2011; iram omni possibili modo preme:
plus una illa te diffamabit, quam multæ virtutes commendabunt

Quiet day at work so I tried upgrading Keycloak on a test server from 11 to 12 to check out the new console.

Well, this has got to be a decent contender for the worst ratio between stack trace length and actual diagnostic usefulness:

https://pastebin.com/raw/hnDcxVke

# ? Dec 29, 2020 15:42

Carbon dioxide: Oct 9, 2012

NihilCredo posted:

Quiet day at work so I tried upgrading Keycloak on a test server from 11 to 12 to check out the new console.

Well, this has got to be a decent contender for the worst ratio between stack trace length and actual diagnostic usefulness:

https://pastebin.com/raw/hnDcxVke

The first line is the actual problem, isn't it?

# ? Dec 29, 2020 16:12

ultrafilter: Aug 23, 2007; It's okay if you have any questions.

https://twitter.com/dabit3/status/1341537427761074177

# ? Dec 29, 2020 16:25

Adbot: ADBOT LOVES YOU

# ? May 14, 2024 11:58

YanniRotten: Apr 3, 2010; We're so pretty,
oh so pretty

Carbon dioxide posted:

The first line is the actual problem, isn't it?

Yep so how would you fix it? This doesn't tell you how to correct the issue, it basically tells you "hey read our source code and try to decipher what thing you control can be changed to avoid the problem."

# ? Dec 29, 2020 16:25

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›1503 »