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

%getenv() and %dirpath return empty #232

Closed
scmdavid opened this issue Jun 23, 2022 · 8 comments · Fixed by #305
Closed

%getenv() and %dirpath return empty #232

scmdavid opened this issue Jun 23, 2022 · 8 comments · Fixed by #305

Comments

@scmdavid
Copy link

Version: 1.2022.5

Test the follow with the server directly

@startuml
!log %version()
!log %getenv("OS")
!log %dirpath()
@enduml

From server stdout

[Log] 1.2022.5
[Log]
[Log]

I would like use the %getenv() to construct the !include, but it returned empty (same for OS, PATH or JAVA_HOME)

@The-Lum
Copy link
Collaborator

The-Lum commented Jun 23, 2022

Hello @scmdavid, and all,

See perhaps this Security Page (that defines security profile and affects the usage of the builtin functions):

If that can help,
Regards.

@scmdavid
Copy link
Author

Hi @The-Lum

Thanks for your info

I set the PLANTUML_SECURITY_PROFILE to UNSECURE from the environment and started the server locally with Maven's Jetty plugin

>echo %PLANTUML_SECURITY_PROFILE%
UNSECURE

>mvn jetty:run "-Djetty.http.port=9999"
[INFO] Scanning for projects...
[INFO]

However, I get the same result

@startuml
!log %version()
!log %dirpath()
!log %getenv("JAVA_HOME")
@enduml
[Log] 1.2022.5
[Log]
[Log]

@HeinrichAD
Copy link
Collaborator

HeinrichAD commented Jun 10, 2023

The reason for %getenv(...) does not work as expected lies here: plantuml code (no PlantUML Server code).

ALLOW_INCLUDE (which is the ALLOW_PLANTUML_INCLUDE environment variable) instead of PLANTUML_SECURITY_PROFILE is checked. If you start plantuml as jar file manually, ALLOW_INCLUDE seems to be set to true by default (see here). As a security feature it is deactivated by default inside the PlantUML server.

Therefore this is working:

java -jar target/plantuml.jar -tpng diagram.txt

while PlantUML server is not. At least as long as ALLOW_PLANTUML_INCLUDE=true is not set as environment variable.
Hence, the work around would be to set the ALLOW_PLANTUML_INCLUDE environment variable to true.

diagram.txt
@startuml
!log %version()
!log %dirpath()
!log %getenv("USER")
@enduml

%dirpath() is not even working via java -jar target/plantuml.jar -DPLANTUML_SECURITY_PROFILE=UNSECURE -tpng diagram.txt.
Only if you set the PLANTUML_SECURITY_PROFILE as OS environment variable it works:
PLANTUML_SECURITY_PROFILE=UNSECURE java -jar target/plantuml.jar -tpng diagram.txt.
For my this seems to be a bug, therefore I created an issue in the plantuml project itself.
See: plantuml/plantuml#1450
If this part is clear, a further investigation could be started.

In general it might be a good idea to set the PLANTUML_SECURITY_PROFILE variable to INTERNET by default for the PlantUML Server. People could always change it back to LEGACY if necessary or add special paths to the allowlist.
More information can be found here: https://plantuml.com/security

Also issues #172 is also related to this.

@arnaudroques I don't know if this is intended but at least for this scenario / use case it is bewildering. I do not know if this function is also used for other stuff, but since it should mainly be there to get the environment value via getenv I thing this function should check the PLANTUML_SECURITY_PROFILE instead of ALLOW_INCLUDE, should it?
Something like if (SecurityUtils.getSecurityProfile() == SecurityProfile.UNSECURE) maybe?
Or does this also have something to do with the !include mechanism?

Edit: Only checking SecurityProfile.UNSECURE does not make much sense. It may be a better idea to check SecurityProfile.UNSECURE or if the environment variable starts with PLANTUML.

proposal
public TValue executeReturnFunction(TContext context, TMemory memory, LineLocation location, List<TValue> values,
		Map<String, TValue> named) throws EaterException, EaterExceptionLocated {
+	final String name = values.get(0).toString();

	// ::comment when __CORE__
-	if (OptionFlags.ALLOW_INCLUDE == false)
+	if (SecurityUtils.getSecurityProfile() != SecurityProfile.UNSECURE && !name.toUpperCase().startsWith("PLANTUML")) {
		// ::done
		return TValue.fromString("");
	}
	// ::comment when __CORE__

-	final String name = values.get(0).toString();
	final String value = getenv(name);
	if (value == null)
		return TValue.fromString("");

	return TValue.fromString(value);
	// ::done
}

@arnaudroques
Copy link
Contributor

I thing this function should check the PLANTUML_SECURITY_PROFILE instead of ALLOW_INCLUDE, should it?

Yes, you are right.

The ALLOW_INCLUDE is an old stuff that was introduced a long time ago to specifically prevent web users to get information from a running PlantUML web server.

We should now use instead the more recent PLANTUML_SECURITY_PROFILE. We certainly will.
We are also going to follow your suggestion to check if the environment variable starts with PLANTUML.

@arnaudroques
Copy link
Contributor

The ALLOW_INCLUDE has been removed in last beta of the core library.
Maybe you can have a look at the code, if that's fine for you.
Next steps could be:

  • publishing a new release 1.2023.9
  • modify plantuml-server, remove use of ALLOW_INCLUDE and default PLANTUML_SECURITY_PROFILE to INTERNET.

Any though?

@HeinrichAD
Copy link
Collaborator

I took a quick look into your code changes and had one question to a specific part. See comment plantuml/plantuml@fbe7fa3#r117852424.

modify plantuml-server, remove use of ALLOW_INCLUDE and default PLANTUML_SECURITY_PROFILE to INTERNET.

Done (inside PR #301). But this PR required PlantUML core 1.2023.9.

So, if you decide to to publish PlantUML core version 1.2023.9 in the future, the next step would be to run the tests of PR #301 again. If these result in no problems, it should be ready to merge.

@HeinrichAD
Copy link
Collaborator

This should now be done with version 1.2023.9

  • You can access any environment variable starting with PLANTUML as prefix except variables starting with plantuml.security any time.
  • You can access all environment variables if the PLANTUML_SECURITY_PROFILE is set to UNSECURE.
  • Likewise, %dirpath() will also only return a value if PLANTUML_SECURITY_PROFILE is set to UNSECURE. Otherwise it will always be empty.
  • It is also possible to include a file over an env variable e.g. !include %getenv("PLANTUML_INCLUDE_TEMPLATE_1") if the plantuml.allowlist.path is set appropriately. Also see https://plantuml.com/security.

Therefore, this issue can now be closed.

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

Successfully merging a pull request may close this issue.

4 participants