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

Taint support #203

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Taint support #203

wants to merge 3 commits into from

Conversation

msteveb
Copy link
Owner

@msteveb msteveb commented Jul 26, 2021

See README.taint for details

========================

Author: Steve Bennett <steveb@workware.net.au>
Date: 24 May 2011
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this really planned 10 years ago?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, really. Been out of tree for that long. I thought it was time to share.
Thanks for your edits below. I'll integrate them.

Perl and Ruby support the concept of tainted data, taint sources
and taint sinks. The idea is to improve security in situations
where data may be coming from outside the program (e.g. input
to a web application) should not inadvertently be output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
to a web application) should not inadvertently be output
to a web application). This data should not inadvertently be output

(to avoid SQL injections attacks) or to execute system commands
(to avoid system attacks).

Standard Tcl does not support tainting. Instead it uses "safe"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Standard Tcl does not support tainting. Instead it uses "safe"
Standard Tcl does not support tainting, but uses "safe"

Comment on lines +73 to +75
While the tainted value can be distinguished from other values
in the container, the container is not tainted. However if the container
needs to change representation (the entire container becomes tainted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
While the tainted value can be distinguished from other values
in the container, the container is not tainted. However if the container
needs to change representation (the entire container becomes tainted.
If the tainted value can be distinguished from other values
in the container, the container is not tainted. However, if the container
needs to change representation, the entire container becomes tainted.

Taint types
-----------
It may be useful to distinguish between different types of taint.
Each taint type is associate with a bit field. The standard taint
Copy link
Contributor

@gromgit gromgit Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Each taint type is associate with a bit field. The standard taint
Each taint type is assigned a bit in a taint bit field. The standard taint


More Information
----------------
In order to simplify taint propagation, the interpreter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In order to simplify taint propagation, the interpreter
To simplify taint propagation, the interpreter


The Rules
---------
- The taint and untaint commands operate on variables and taint/untaint the contents of the variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The taint and untaint commands operate on variables and taint/untaint the contents of the variable
- The taint and untaint commands operate on variables, and taint/untaint the contents of the variable

The Rules
---------
- The taint and untaint commands operate on variables and taint/untaint the contents of the variable
- Adding/modifying a list/dict/array element taints that element plus the "container" but not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Adding/modifying a list/dict/array element taints that element plus the "container" but not
- Adding/modifying a list/dict/array element taints that element plus the "container", but not

Specific Notes
--------------
In general, a conservative approach is used to tainting, so if
a command creates a new object while any of it's arguments are tainted,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
a command creates a new object while any of it's arguments are tainted,
a command creates a new object while any of its arguments are tainted,

a command creates a new object while any of it's arguments are tainted,
the new object is also tainted.

However the list-related commands are more intelligent.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
However the list-related commands are more intelligent.
However, the list-related commands are more intelligent.


However the list-related commands are more intelligent.
All list-related commands such as lindex, lrange, lassign and lreplace will
maintain the taint of existing list elements, but will avoid tainting untainted elements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
maintain the taint of existing list elements, but will avoid tainting untainted elements.
not change the taint of existing list elements.

@dbohdan
Copy link
Contributor

dbohdan commented Jul 26, 2021

I have only played with this feature in the REPL so far, but I can see it being useful. Some feedback:

  • It might help if tainted data errors had a specific errorcode, so you could distinguish them not just by message.
  • info tainted without an argument returns 1. Unless I am missing something, it should be an error.

@msteveb
Copy link
Owner Author

msteveb commented Jul 26, 2021

Thanks. All feedback is useful. I am hesitant about adding new error codes, but I'll consider yet.
Yes, info tainted with no arg should be an error.

@dbohdan
Copy link
Contributor

dbohdan commented Jul 26, 2021

Thanks.

You're welcome!

I am hesitant about adding new error codes, but I'll consider yet.

Why? I understand that there is no central accounting for -errorcodes, so the effect of adding new ones will be between a command and its users. I can't see any negative impact from it.

Yes, info tainted with no arg should be an error.

Ah, good.

@dbohdan
Copy link
Contributor

dbohdan commented Jul 26, 2021

I have a feature request: check for tainted data in the Redis extension, too, and in Win32_ShellExecute in jim-win32.c.

@msteveb
Copy link
Owner Author

msteveb commented Jul 29, 2021

Yes, for sure with Win32_ShellExecute. Not so sure about the redis extension, as there is no quoting in args passed to redis, it is hard to get an injection attack. We could check for taint in the command (first arg) to redis, as that certainly shouldn't come from outside the system, but this seems like a low risk.

Regarding -errorcode, my apology - I confused this with return codes. Error codes aren't particularly well supported in Jim Tcl at the moment. Basically exec will set $errorCode which will be picked up by try/catch to set -errorcode, but nothing else sets $errorCode. I'm open to concrete suggestions.

@dbohdan
Copy link
Contributor

dbohdan commented Jul 29, 2021

Yes, for sure with Win32_ShellExecute.

Great! \o/

Not so sure about the redis extension, as there is no quoting in args passed to redis, it is hard to get an injection attack. We could check for taint in the command (first arg) to redis, as that certainly shouldn't come from outside the system, but this seems like a low risk.

There is also the command EVAL, which evaluates its argument as Lua code, and I can think of several others where one would pretty much only pass something from the user to them by mistake. I think it is not worth playing Whac-A-Mole trying to blacklist sensitive arguments to all of them. (Blacklists are a weak approach to security: new dangerous commands may be added later, in forks, or in other software that speaks the Redis protocol.) "Check nothing", "only check the command argument", and "check all arguments" are probably the only viable options.

Regarding -errorcode, my apology - I confused this with return codes. Error codes aren't particularly well supported in Jim Tcl at the moment. Basically exec will set $errorCode which will be picked up by try/catch to set -errorcode, but nothing else sets $errorCode. I'm open to concrete suggestions.

Ah, I see. I'd set the -errorcode $errorCode to TAINTED or TAINTEDDATA in Jim_SetTaintError.

Copy link
Contributor

@dbohdan dbohdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Welcome to Jim version 0.80
. source jimlib.tcl
. set a 5
5
. taint a
. lib::try { exec echo $a } trap TAINTED {x y} { list $x $y }
{exec: tainted data} {-code 1 -level 0 -errorinfo {} -errorcode TAINTED}

@msteveb
Copy link
Owner Author

msteveb commented Aug 2, 2021

Thanks. Will merge once I've updated the documentation.

@@ -0,0 +1,143 @@
Taint Suport for Jim Tcl
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Taint Suport for Jim Tcl
Taint Support for Jim Tcl

@msteveb
Copy link
Owner Author

msteveb commented Mar 4, 2023

Thanks. Now that 0.82 is released I plan to get this into the next release

See README.taint

Signed-off-by: Steve Bennett <steveb@workware.net.au>
Signed-off-by: Steve Bennett <steveb@workware.net.au>
Compile tested only.

Signed-off-by: Steve Bennett <steveb@workware.net.au>
@msteveb
Copy link
Owner Author

msteveb commented Aug 27, 2023

FYI, I plan to merge this as part of https://github.com/msteveb/jimtcl/tree/cmd-register in the coming weeks

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 this pull request may close these issues.

None yet

4 participants