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

Issue I. User Input Written to Logs #829

Closed
fpietrosanti opened this Issue Mar 3, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@fpietrosanti
Copy link
Contributor

fpietrosanti commented Mar 3, 2014

No description provided.

@fpietrosanti fpietrosanti added this to the LeastAuthorityPentest milestone Mar 3, 2014

@fpietrosanti

This comment has been minimized.

Copy link
Contributor

fpietrosanti commented Mar 3, 2014

All messages should be logged in a way that safely and unambiguously encodes non-printable characters. All logging paths should go through the same, safe sanitizer.

evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Apr 9, 2014

@evilaliv3

This comment has been minimized.

Copy link
Contributor

evilaliv3 commented Apr 9, 2014

@defuse :

in order to address the issue we have:

@defuse

This comment has been minimized.

Copy link

defuse commented Apr 9, 2014

@evilaliv3 In the code in the report, the return values are prefixed with ': ' or 'binary: ' or 'utf-8: ' so that the result is unambiguous. Those have been removed from the method that was put in globaleaks. I think it is better to be unambiguous, but I'm not sure if it will actually matter in practice. It would certainly help if someone was writing a script to decode the logs.

Other than that it looks good.

@evilaliv3

This comment has been minimized.

Copy link
Contributor

evilaliv3 commented Apr 9, 2014

thank you @defuse for the fast response. we will evaluate this possibility but i've done it intentionally in order to flatten out all in the same format.

evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Apr 11, 2014

hellais added a commit to globaleaks/GLBackend-outdated that referenced this issue Apr 11, 2014

Merge branch 'fix_issue_829' of github.com:globaleaks/GLBackend into …
…fix_issue_829

* 'fix_issue_829' of github.com:globaleaks/GLBackend:
  addressed issue globaleaks/GlobaLeaks#829

Conflicts:
	globaleaks/utils/utility.py

hellais added a commit to globaleaks/GLBackend-outdated that referenced this issue Apr 11, 2014

@evilaliv3

This comment has been minimized.

Copy link
Contributor

evilaliv3 commented Apr 14, 2014

allright also this ticket can be considered closed.

the logs do now filter against terminal escape sequences and not visible characters in general and a complete review of the error messages has been done.

@evilaliv3 evilaliv3 closed this Apr 14, 2014

@darius

This comment has been minimized.

Copy link

darius commented Apr 17, 2014

Checking the new log_remove_escapes code: it calls str() on the argument, and catches Exception, and in that case returns "Failur in log_remove_escapes %s" % e. If the argument is of a class with str, which raises an exception, and the exception has a str producing something dangerous, then this code fails to escape it. If it used %r instead of %s, I'd be happier, because the normal behavior of %s is a non-escaped string, which the normal result with %r is escaped. It's still possible to override that, but not so likely as an accident.

About the disambiguating prefixes, I'd suggest tailoring them to what's common and nice-looking in your system instead of eliminating them, though like @defuse I'm not sure it'll matter.

@darius

This comment has been minimized.

Copy link

darius commented Apr 17, 2014

Alternatively (or, really, in addition), in the failure case I complained about above, you could test the result string for any non-printable ASCII characters, and substitute a last-ditch truly-safe complaint message, if raising an exception is no good. All the other code paths are OK without such a check.

@evilaliv3

This comment has been minimized.

Copy link
Contributor

evilaliv3 commented Apr 17, 2014

ok for the remediation stage i'm going to simply apply your suggestion for the change from "%s" to "%r". thanks for your review.

evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Apr 17, 2014

evilaliv3 added a commit to globaleaks/GLBackend-outdated that referenced this issue Apr 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment