Arbitrary User Input? #357

2br-2b · 2024-06-13T22:52:52Z

I'm curious - what's pydot's policy on data sanitization? Should I trust pydot to sanitize fields (like the label) or should I do that myself?

Sanitizing data is always a good idea anyway, but I just wanted to check

lkk7 · 2024-06-14T08:08:18Z

I think it's safe to assume for the user that we don't sanitize input at all, as pydot is just a wrapper.
Maybe it's a good idea to put this information in the README.

Do you have any idea of how this could be exploited?

2br-2b · 2024-06-14T21:56:39Z

That's a good assumption to know about. Maybe it should go in secuirty.md? I think that's a matter of preference, unless you put it somewhere near the bottom of the readme.

pydot/src/pydot/core.py

Lines 218 to 226 in d8645e7

    
8000
    
           process = subprocess.Popen( 
        
               program_with_args, 
        
               env=env, 
        
               cwd=working_dir, 
        
               shell=False, 
        
               stderr=subprocess.PIPE, 
        
               stdout=subprocess.PIPE, 
        
               **kwargs, 
        
           )

was what I was most worried about, but per https://stackoverflow.com/a/52022409/10442089 , this should be safe from injection

lkk7 · 2024-06-15T09:31:55Z

Oh, that's what you meant. If I remember correctly, there is no user input in that command, all parameters are "fixed".
But still, it's a good thing to look at and document.

2br-2b · 2024-06-16T00:40:05Z

Yeah, my current use case is a adding this to my storywriting Discord bot. I'm planning on using this to let users create character graphs to keep track of what's going on. Given how insane the stories generally end up being (the bot chooses random server members and to continue writing one big story, so it bounces around a bunch), I think this will be a huge help for a lot of servers:

My concern is whether allowing users arbitrary access to create custom labels will open me up to command injection, or whether I should sanitize user input before passing it to Graphviz

I haven't yet found any exploits, but I figured better safe than sorry, so I reached out here to get a second opinion and see if the library is designed with this in mind or if I should handle sanitization myself

This library has been doing a quite a bit of heavy lifting for that feature btw! You (along with the past maintainers and the Graphviz developers) have done a great job with this library!

ferdnyc · 2024-06-16T05:30:04Z

I think you should be "as safe as graphviz is" from the possibility of code injection, which should be pretty safe.

The subprocess call, as you noted, is safe (and as @lkk7 said, it includes no user input in the arguments).

And even graphviz's "HTML-like labels" (which we don't even parse other than as opaque string values, so there's no chance of us executing anything untoward) — are referred to that way specifically because they implement, "by hand" in the graphviz source, only a very small subset of HTML. Nobody's going to manage any script injections or anything like that, simply because it doesn't support any scripting at all, and will cheerfully ignore any tags other than the dozen or so it supports. The most advanced markup available is <TABLE>.

2br-2b mentioned this issue Jun 18, 2024

HTML tags #366

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Arbitrary User Input? #357

Arbitrary User Input? #357

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arbitrary User Input? #357

Arbitrary User Input? #357

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!