Ticket #631 (assigned defect)

Opened 2 years ago

Last modified 21 months ago

IRC bot gives 'ValueError' when fed a singlequote

Reported by: tycho Owned by: ddunbar
Priority: major Milestone: 0.8.+
Version: 0.7.11 Keywords: irc
Cc: daniel@…

Description

Example:

<neunon> CrawlBuild?, force build crawl-debug-x86_64-centos attempt to bomb with rvollmert's latest changes <CrawlBuild?> Something bad happened (see logs): <type 'exceptions.ValueError?'>

Gives:

2009-10-22 00:45:22-0700 [IrcStatusBot?,client] irc command force 2009-10-22 00:45:22-0700 [IrcStatusBot?,client] Unhandled Error

Traceback (most recent call last):

File "/usr/lib/python2.6/site-packages/twisted/words/protocols/irc.py", line 1484, in lineReceived

self.handleCommand(command, prefix, params)

File "/usr/lib/python2.6/site-packages/twisted/words/protocols/irc.py", line 1496, in handleCommand

method(prefix, params)

File "/usr/lib/python2.6/site-packages/twisted/words/protocols/irc.py", line 1041, in irc_PRIVMSG

self.privmsg(user, channel, message)

File "/usr/lib/python2.6/site-packages/buildbot/status/words.py", line 724, in privmsg

contact.handleMessage(message, user)

--- <exception caught here> ---

File "/usr/lib/python2.6/site-packages/buildbot/status/words.py", line 635, in handleMessage

meth(args.strip(), who)

File "/usr/lib/python2.6/site-packages/buildbot/status/words.py", line 380, in command_FORCE

args = shlex.split(args) # TODO: this requires python2.3 or newer

File "/usr/lib/python2.6/shlex.py", line 279, in split

return list(lex)

File "/usr/lib/python2.6/shlex.py", line 269, in next

token = self.get_token()

File "/usr/lib/python2.6/shlex.py", line 96, in get_token

raw = self.read_token()

File "/usr/lib/python2.6/shlex.py", line 172, in read_token

raise ValueError?, "No closing quotation"

exceptions.ValueError?: No closing quotation

Should I be reporting this to Twisted or can you guys work around this?

And uh, it looks to me like this is a route to a security hole if it's not escaping the singlequote.

Change History

comment:1 Changed 2 years ago by ipv6guru

  • Milestone changed from undecided to 0.7.+

Just confirming that this is reproducible every time. Would be nice to see a fix for the next 0.7.11 release.

comment:2 Changed 2 years ago by dustin

  • Milestone changed from 0.7.+ to 0.7.12

It's not a security problem - shlex is just string.split() on steriods.

This error should be handled more smoothly, but that just means presenting a more useful error message via IRC. The fix is to not use unevenly-quoted strings in the IRC bot:

CrawlBuild force build crawl-debug-x86_64-centos "attempt to bomb with rvollmert's latest changes"

comment:3 Changed 2 years ago by ddunbar

Why do we use shlex anyway? Are we are only using shlex so we can parse "foo bar" as a single token?

It seems an unintuitive interface for an IRC bot. Should we just reimplement the command tokenization to be robust?

comment:4 Changed 2 years ago by ddunbar

  • Cc daniel@… added

comment:5 Changed 2 years ago by dustin

That's basically all shlex does, is parse quotes into an arglist. The problem is that we don't use it everywhere in IRC - only for some commands.

comment:6 Changed 2 years ago by ddunbar

The problem is shlex rules allow it to fail, which isn't ideal in an IRC bot. I expect the syntax to be

force build foo any text I want

not

force build foo "any text I want"

When do we need to allow things to be quoted? The only options I can think off are builder names (and the like) that had spaces.

Looking at the code, I'd be tempted to just completely redo how the IRC bot does command parsing.

comment:7 Changed 2 years ago by dustin

  • Owner set to ddunbar
  • Status changed from new to assigned

Sounds good to me. I disagree about not needing quotes, tho - I do *not* like terminating things at EOL. It's so DOS.

comment:8 Changed 2 years ago by dustin

  • Milestone changed from 0.7.12 to 0.8.0

This is not release-critical, since it is expected (if odd) behavior.

comment:9 Changed 2 years ago by dustin

  • Milestone changed from 0.8.0 to 0.8.+

comment:10 Changed 21 months ago by dustin

  • Keywords irc added
Note: See TracTickets for help on using tickets.