4 ms·

The entire point of Velocity templates is to allow the direct use of Java objects in your template. It has to execute code. It's fair to question why it was exp

The entire point of Velocity templates is to allow the direct use of Java objects in your template. It has to execute code. It's fair to question why it was exposed to an unauthenticated user sign up form, but the general reason Confluence uses it is to enable the development of plugins and user macros that allow server owners to add custom functionality on top of base Confluence. The dubious sanitizing looks pretty bad, though. At bare minimum, Confluence should be executing in a sandbox that doesn't allow to call the exec method on the runtime instance. I can't imagine why Confluence would ever need shell access. Pretty good reason to run self-hosted Confluence in a distroless container with access to nothing but a JVM and whatever config files it requires. A scarier thing is I believe Confluence, and all Atlassian products, store their DB credentials in a plain-text .properties file with a well-known standard path. Instead of trying to get a shell, this code could have just tried to read the expected path and print out the contents and it's pretty likely you could get a raw connection to the Confluence database.

mhitza 5y ago
To some extent I think that's true of any templating system in a language with object oriented support. I don't use Java to be familiar with how the underlying OGNL library does things; it's obviously there to make property access patterns and chaining on objects easier within template languages. But I think a thing like this is easy to pass a code review because the difference between $ and $! is minimal. If it had a name like eval it would be easier to spot and make a reviewer think for a second. That would be more likely to be caught even in an environment where there is the pressing urge to push out code fast. I can only assume that a developer that is not in a hurry could have done something like (inventing Velocity template syntax here) $someobject.handle($queryString) Where the underlying object associated with $someobject has a handle method with a switch statement that based $queryString values does the proper property chain access. As much as PHP is harped on so often, this was the most important lesson PHP developers had to learn early, to run a switch statement on query strings when including files. (in the days before routing was a solved problem)

mhitza 5y ago

To some extent I think that's true of any templating system in a language with object oriented support. I don't use Java to be familiar with how the underlying OGNL library does things; it's obviously there to make property access patterns and chaining on objects easier within template languages. But I think a thing like this is easy to pass a code review because the difference between $ and $! is minimal. If it had a name like eval it would be easier to spot and make a reviewer think for a second. That would be more likely to be caught even in an environment where there is the pressing urge to push out code fast. I can only assume that a developer that is not in a hurry could have done something like (inventing Velocity template syntax here) $someobject.handle($queryString) Where the underlying object associated with $someobject has a handle method with a switch statement that based $queryString values does the proper property chain access. As much as PHP is harped on so often, this was the most important lesson PHP developers had to learn early, to run a switch statement on query strings when including files. (in the days before routing was a solved problem)