carlos-montiers / enhancedbatch

Enhances your windows command prompt https://www.enhancedbatch.com
Other
5 stars 1 forks source link

Protect dynamic pseudo environment variables - Don't allow SET to override them #42

Open DaveBenham opened 4 years ago

DaveBenham commented 4 years ago

I think it would be good if EB could prevent SET from defining an environment variable that would override any of the pseudo environment variables. It could be patched to fail and raise an error if an attempt was made to set the value. There could be a @ProtectEnvironment extension that could control the behavior: 0 = allow override, 1= disallow override. I'm thinking the default would be 1 upon EB load.

It might also be good if EB explicitly clears any existing dynamic variable overrides upon load and whenever the @ProtectEnvironment state changes from 0 to 1.

There are also true environment variables that can be overridden, but perhaps shouldn't. Perhaps they should also be protected, or @ProtectEnvironment could have additional states: 0 = no protection, 1 = protect dynamic variables, 2 = protect predefined constant variables

Implementation could be a bit tricky for SET /A, given that it can define many environment variables with a single statement.

Here is a list of dynamic variables I think definitely should be protected:

CD CMDEXTVERSION CMDCMDLINE DATE ERRORLEVEL FIRMWARETYPE HighestNumaNodeNumber RANDOM TIME __CD__ (two underscores at beginning and end) __APPDIR_\ (two underscores at beginning and end)

Here are true environment variables that perhaps should be constant, and are candidates to be protected. Obviously these cannot be cleared when protection is enabled:

ALLUSERSPROFILE APPDATA ClientName CommonProgramFiles CommonProgramFiles(x86) COMPUTERNAME COMSPEC HOMEDRIVE HOMEPATH LOCALAPPDATA LOGONSERVER NUMBER_OF_PROCESSORS OS PROCESSOR_ARCHITECTURE PROCESSOR_ARCHITEW6432 PROCESSOR_IDENTIFIER PROCESSOR_LEVEL PROCESSOR_REVISION ProgramW6432 ProgramData ProgramFiles PSModulePath PUBLIC SESSIONNAME SYSTEMDRIVE SYSTEMROOT UserDNSDomain USERDOMAIN USERDOMAIN_ROAMINGPROFILE USERNAME USERPROFILE WINDIR

carlos-montiers commented 4 years ago

Also TEMP and TMP I think it can be done, but maybe is too much overhead check all the list every time you set a variable. Some time ago I suggest create the concept of "system variables" storing in the heap variables like $WINDIR, $USERNAME, read only, but @adoxa downvote it because is copy from the environment to the heap. Maybe is more simple document that variables and encourage not overwrite that.

DaveBenham commented 4 years ago

The problem comes when you call a script that you didn't write, and it overwrites one of the dynamic variables without you knowing it. If EB is loaded and this is implemented, then you don't have to worry about using such a script.

TEMP and TMP should be writable - It is perfectly reasonable to change the temporary folder location within a script.

It may be possible that there are legitimate reasons for overwriting some of the true environment variables in my second list. That is why I listed them as candidates - I'm not sure if it is appropriate.

But the pseudo environment variables should never be overridden. That list is pretty small, so should not be a significant performance hit.

adoxa commented 4 years ago

No thoughts on this, yet, but a couple of notes.

FIRMWARE_TYPE

Wrong list, it's not a CMD variable.

__CD__ __APPDIR__ NUMBER_OF_PROCESSORS

These are system pseudo-variables and cannot be overridden, anyway.

DaveBenham commented 4 years ago

I already knew that about __CD__ and __APPDIR__

But nice to know about the NUMBER_OF_PROCESSORS, thanks.

The same is true for FIRMWARE_TYPE - it cannot be overridden.

At least for __CD__ and __APPDIR__, they can be overridden on XP. I wonder about NUMBER_OF_PROCESSORS and FIRMWARE_TYPE.

Even when it can't be overridden, it would be useful to raise an error if an attempt is made to do so, otherwise a user might try to override the value, assume it worked, and then wonder what is going wrong later on. I think better to raise an error and let the user know right away that the SET is not working (non-functional)

adoxa commented 4 years ago

At least for __CD__ and __APPDIR__, they can be overridden on XP. I wonder about NUMBER_OF_PROCESSORS and FIRMWARE_TYPE.

NUMBER_OF_PROCESSORS can on XP, but not on 7 (these variables still exist in the environment block, but the dynamic value is always returned, so there's a potential discrepancy, too; that's the same reason heap variables don't work with /A); FIRMWARE_TYPE doesn't exist on either.

DaveBenham commented 4 years ago

Another option is to implement an @ getter extension for all of the dynamic pseudo environment variables as discussed at #43. That would be immune from any environment variable overrides, and this request becomes somewhat moot.

adoxa commented 4 years ago

I'm not going to do that, so it's protection (which is relatively easy) or nothing.

carlos-montiers commented 4 years ago

I vote up to protect these next dynamic variables:

CD
CMDCMDLINE
DATE
ERRORLEVEL
RANDOM
TIME

... if it will not overhead badly each set operation