debase64 does not escape newline (0x0a)

[ghost]

Text: Foobar\nBarfoo Hex: 466f6f6261720a426172666f6f

13:25 <+aydiosmio> .debase64 Rm9vYmFyCkJhcmZvbw== 13:25 <+CloudBot> (aydiosmio) Foobar 13:25 <+CloudBot> Barfoo

[foxlet]

I'd say that would be intended behavior unless there is a security risk related to it.

[ghost]

The behavior allows a user to cause the bot to generate numerous lines of output, which risks flooding the channel, and causing the bot to be banned from the channel if enforced or removed from the server for excess flood.

[ryanohoro]

11:33 AM <@aydiosmio> .debase64 SQphbQp0aGUKdmVyeQptb2RlbApvZgphCm1vZGVybgptYWpvcgpnZW5lcmFs
11:33 AM <+JARVIS> (aydiosmio) I
11:33 AM <+JARVIS> am
11:33 AM <+JARVIS> the
11:33 AM <+JARVIS> very
11:33 AM <+JARVIS> model
11:33 AM <+JARVIS> of
11:33 AM <+JARVIS> a
11:33 AM <+JARVIS> modern
11:33 AM <+JARVIS> major
11:33 AM <+JARVIS> general

[ryanohoro]

New problem regarding this issue. A specially crafted base64 string has been demonstrated to cause cloudbot to quit due to excess flood. The following base64 string is composed primarily of alternating spaces and newlines.

<testuser> [15:48:54] .debase CmhheAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAKIAogCiAK
<*buffextras> [15:48:54] JARVIS!jarvis@just.a.rather.very.intelligent.system quit with message: [Excess Flood]

[tuxxy]

What behavior would be considered appropriate for handling newlines? Should the bot owner choose whether or not to allow newlines, or show the string up until the first newline, or just simply throw it away and return nothing?

[ctian1]

I believe this was fixed in 375dd6c, wasn't it?

[daboross]

Still occurs in today's cloudbot. @foxlet what would you think of escaping \n if there are more than three newlines in the output?

[tuxxy]

@daboross Even with escaping it for > 3 newlines, there is a potentional for DoS by simply performing the command repeatedly until it gets kicked for flooding, assuming CloudBot responds in time.

Why not replace the newlines with a space and respond as if they weren't there?

[daboross]

@tuxxy I guess allowing any multiple lines does make it vulnerable - not allowing any is fine with me.

I was thinking of showing them as \n rather than just because as a utility command, I would think it'd be more useful to get a more full idea of what is contained in the base64 code, rather than hiding the newlines to display better. I guess it's just a matter of what the primary usecase of debase64 is, if there even is one.

[tuxxy]

I honestly don’t think there is anything useful with displaying newlines. All it does is make the text less readable.

In most cases, coming from security and software engineering, I don’t see a case where the base64 (being posted in IRC) will ever be so important to keep formatting. If it’s that important, there are more appropriate tools to use than an IRC bot.

That’s just my two cents. :P

On Jun 21, 2016, at 17:39, Dabo Ross notifications@github.com wrote:

@tuxxy https://github.com/tuxxy I guess allowing any multiple lines does make it vulnerable - not allowing any is fine with me.

I was thinking of showing them as \n rather than just because as a utility command, I would think it'd be more useful to get a more full idea of what is contained in the base64 code, rather than hiding the newlines to display better. I guess it's just a matter of what the primary usecase of debase64 is, if there even is one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227604156, or mute the thread https://github.com/notifications/unsubscribe/ADbn7o0-R0_8Wl9s2ztvhxFsFroEkVzsks5qOHZCgaJpZM4EW3s1.

[knight-ryu12]

I think we could make result NOT putting on IRC. instead, make it hatebin? 2016/06/22 8:55 "Tux" notifications@github.com:

I honestly don’t think there is anything useful with displaying newlines. All it does is make the text less readable.

In most cases, coming from security and software engineering, I don’t see a case where the base64 (being posted in IRC) will ever be so important to keep formatting. If it’s that important, there are more appropriate tools to use than an IRC bot.

That’s just my two cents. :P

On Jun 21, 2016, at 17:39, Dabo Ross notifications@github.com wrote:

@tuxxy https://github.com/tuxxy I guess allowing any multiple lines does make it vulnerable - not allowing any is fine with me.

I was thinking of showing them as \n rather than just because as a utility command, I would think it'd be more useful to get a more full idea of what is contained in the base64 code, rather than hiding the newlines to display better. I guess it's just a matter of what the primary usecase of debase64 is, if there even is one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227604156>, or mute the thread < https://github.com/notifications/unsubscribe/ADbn7o0-R0_8Wl9s2ztvhxFsFroEkVzsks5qOHZCgaJpZM4EW3s1 .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227606546, or mute the thread https://github.com/notifications/unsubscribe/ALa_E_j9yhWqy5wqPTlaW8qIBvhTC__Sks5qOHn9gaJpZM4EW3s1 .

[tuxxy]

For multiple lines, or for all decoded base64?

On Jun 21, 2016, at 19:32, Chromaryu notifications@github.com wrote:

I think we could make result NOT putting on IRC. instead, make it hatebin? 2016/06/22 8:55 "Tux" notifications@github.com:

I honestly don’t think there is anything useful with displaying newlines. All it does is make the text less readable.

In most cases, coming from security and software engineering, I don’t see a case where the base64 (being posted in IRC) will ever be so important to keep formatting. If it’s that important, there are more appropriate tools to use than an IRC bot.

That’s just my two cents. :P

On Jun 21, 2016, at 17:39, Dabo Ross notifications@github.com wrote:

@tuxxy https://github.com/tuxxy I guess allowing any multiple lines does make it vulnerable - not allowing any is fine with me.

I was thinking of showing them as \n rather than just because as a utility command, I would think it'd be more useful to get a more full idea of what is contained in the base64 code, rather than hiding the newlines to display better. I guess it's just a matter of what the primary usecase of debase64 is, if there even is one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227604156>, or mute the thread < https://github.com/notifications/unsubscribe/ADbn7o0-R0_8Wl9s2ztvhxFsFroEkVzsks5qOHZCgaJpZM4EW3s1 .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227606546, or mute the thread https://github.com/notifications/unsubscribe/ALa_E_j9yhWqy5wqPTlaW8qIBvhTC__Sks5qOHn9gaJpZM4EW3s1 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227619697, or mute the thread https://github.com/notifications/unsubscribe/ADbn7vbvf06yXAKKhQiS0nnuxrM2_q-Xks5qOJCngaJpZM4EW3s1.

[knight-ryu12]

all decoded b64. 2016/06/22 10:34 "Tux" notifications@github.com:

For multiple lines, or for all decoded base64?

On Jun 21, 2016, at 19:32, Chromaryu notifications@github.com wrote:

I think we could make result NOT putting on IRC. instead, make it hatebin? 2016/06/22 8:55 "Tux" notifications@github.com:

I honestly don’t think there is anything useful with displaying newlines. All it does is make the text less readable.

In most cases, coming from security and software engineering, I don’t see a case where the base64 (being posted in IRC) will ever be so important to keep formatting. If it’s that important, there are more appropriate tools to use than an IRC bot.

That’s just my two cents. :P

On Jun 21, 2016, at 17:39, Dabo Ross notifications@github.com wrote:

@tuxxy https://github.com/tuxxy I guess allowing any multiple lines does make it vulnerable - not allowing any is fine with me.

I was thinking of showing them as \n rather than just because as a utility command, I would think it'd be more useful to get a more full idea of what is contained in the base64 code, rather than hiding the newlines to display better. I guess it's just a matter of what the primary usecase of debase64 is, if there even is one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227604156 , or mute the thread <

https://github.com/notifications/unsubscribe/ADbn7o0-R0_8Wl9s2ztvhxFsFroEkVzsks5qOHZCgaJpZM4EW3s1

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227606546 , or mute the thread < https://github.com/notifications/unsubscribe/ALa_E_j9yhWqy5wqPTlaW8qIBvhTC__Sks5qOHn9gaJpZM4EW3s1

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227619697>, or mute the thread < https://github.com/notifications/unsubscribe/ADbn7vbvf06yXAKKhQiS0nnuxrM2_q-Xks5qOJCngaJpZM4EW3s1 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227619864, or mute the thread https://github.com/notifications/unsubscribe/ALa_E221EHAkfGRGORIBjXX0lGVvVm5Bks5qOJEMgaJpZM4EW3s1 .

[tuxxy]

Well, that’s one option, I’m hesitant to agree to it because of such a drastic change, but it’s certainly an option.

On Jun 21, 2016, at 19:34, Chromaryu notifications@github.com wrote:

all decoded b64. 2016/06/22 10:34 "Tux" notifications@github.com:

For multiple lines, or for all decoded base64?

On Jun 21, 2016, at 19:32, Chromaryu notifications@github.com wrote:

I think we could make result NOT putting on IRC. instead, make it hatebin? 2016/06/22 8:55 "Tux" notifications@github.com:

I honestly don’t think there is anything useful with displaying newlines. All it does is make the text less readable.

In most cases, coming from security and software engineering, I don’t see a case where the base64 (being posted in IRC) will ever be so important to keep formatting. If it’s that important, there are more appropriate tools to use than an IRC bot.

That’s just my two cents. :P

On Jun 21, 2016, at 17:39, Dabo Ross notifications@github.com wrote:

@tuxxy https://github.com/tuxxy I guess allowing any multiple lines does make it vulnerable - not allowing any is fine with me.

I was thinking of showing them as \n rather than just because as a utility command, I would think it'd be more useful to get a more full idea of what is contained in the base64 code, rather than hiding the newlines to display better. I guess it's just a matter of what the primary usecase of debase64 is, if there even is one.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <

https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227604156 , or mute the thread <

https://github.com/notifications/unsubscribe/ADbn7o0-R0_8Wl9s2ztvhxFsFroEkVzsks5qOHZCgaJpZM4EW3s1

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227606546 , or mute the thread < https://github.com/notifications/unsubscribe/ALa_E_j9yhWqy5wqPTlaW8qIBvhTC__Sks5qOHn9gaJpZM4EW3s1

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227619697>, or mute the thread < https://github.com/notifications/unsubscribe/ADbn7vbvf06yXAKKhQiS0nnuxrM2_q-Xks5qOJCngaJpZM4EW3s1 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227619864, or mute the thread https://github.com/notifications/unsubscribe/ALa_E221EHAkfGRGORIBjXX0lGVvVm5Bks5qOJEMgaJpZM4EW3s1 .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227619950, or mute the thread https://github.com/notifications/unsubscribe/ADbn7paNMPJa13VSlkqkgjTdzUxoYJCcks5qOJE3gaJpZM4EW3s1.

[Red-M]

This plugin has actually brought up conversation higher up in how cloudbot handles \n in returned output in plugins.

For this reason you can see https://github.com/CloudBotIRC/CloudBot/blob/master/cloudbot/clients/irc.py#L149-L153 for further details.

[tuxxy]

Mind sharing that conversation here?

On Jun 21, 2016, at 22:41, Red_M notifications@github.com wrote:

This plugin has actually brought up conversation higher up in how cloudbot handles \n in returned output in plugins.

For this reason you can see https://github.com/CloudBotIRC/CloudBot/blob/master/cloudbot/clients/irc.py#L149-L153 https://github.com/CloudBotIRC/CloudBot/blob/master/cloudbot/clients/irc.py#L149-L153 for further details.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227641488, or mute the thread https://github.com/notifications/unsubscribe/ADbn7nrkt8oPwKxnLhgRl63BSjlHHcOYks5qOL0HgaJpZM4EW3s1.

[Red-M]

All that would be required is to change https://github.com/CloudBotIRC/CloudBot/blob/master/plugins/utility.py#L144 from a return to a bot.message() call.

The conversation has long since come and gone and can actually be viewed in the the commit history for /cloudbot/clients/irc.py

[tuxxy]

Can we expect to see this obvious security issue be changed, though? The users should not have to rewrite default code to fix something like this.

[knight-ryu12]

Why not? Users can modify own code. at own risk

[Red-M]

If I do so remember correctly it needs to allow for "legacy" plugins to still have new lines. I fully disagree with this action and plugins should be limited to not sending a \n directly due to injection attacks such as the one pointed out here.

My original thinking with my commit to patch the injection allowing ANY IRC action to be sent as the bot is simply disallow stupidity at all cost. I don't think allowing legacy plugins to abuse \n is a good thing in any sense.

[tuxxy]

But this presents a security issue by default. CloudBot shouldn’t take a lazy view of security by default. If they want this behavior, they can do the opposite of what was suggested.

On Jun 21, 2016, at 22:48, Chromaryu notifications@github.com wrote:

Why not? Users can modify own code. at own risk — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227642276, or mute the thread https://github.com/notifications/unsubscribe/ADbn7ng4dUfBuluYA7U8BR1_bezLj4M6ks5qOL6zgaJpZM4EW3s1.

[knight-ryu12]

What? I mean, own instance. Not Pushing to git. 2016/06/22 13:50 "Tux" notifications@github.com:

But this presents a security issue by default. CloudBot shouldn’t take a lazy view of security by default. If they want this behavior, they can do the opposite of what you suggested.

On Jun 21, 2016, at 22:48, Chromaryu notifications@github.com wrote:

Why not? Users can modify own code. at own risk — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227642276>, or mute the thread < https://github.com/notifications/unsubscribe/ADbn7ng4dUfBuluYA7U8BR1_bezLj4M6ks5qOL6zgaJpZM4EW3s1 .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227642573, or mute the thread https://github.com/notifications/unsubscribe/ALa_E-NLmJv_Hp1Oah5WFRqHeQshXDQgks5qOL8egaJpZM4EW3s1 .

[Red-M]

@knight-ryu12 See @foxlet for reasons why it is in its current state.

@tuxxy My original commit fixed IRC action injection and "broke legacy" plugins that abused \n in messages to get multilines. I wanted more security with less stupidity in plugins, as in its forced and you can't wiggle around it. With Cloudbot in its current state this is the quickest and easiest way to fix this. There is no "by default" security flaw, the defaults in bot.message() actually clean out \ns and delete them before sending to IRC.

[knight-ryu12]

ok. I'll do. 2016/06/22 13:55 "Red_M" notifications@github.com:

@knight-ryu12 https://github.com/knight-ryu12 See @foxlet https://github.com/foxlet for reasons why it is in its current state.

@tuxxy https://github.com/tuxxy My original commit fixed IRC action injection and "broke legacy" plugins that abused \n in messages to get multilines. I wanted more security with less stupidity in plugins, as in its forced and you can't wiggle around it. With Cloudbot in its current state this is the quickest and easiest way to fix this. There is no "by default" security flaw, the defaults in bot.message() actually clean out \ns and delete them before sending to IRC.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227643426, or mute the thread https://github.com/notifications/unsubscribe/ALa_E4ZI9BKZ0gngfin8IWieqsxu4_MMks5qOMA4gaJpZM4EW3s1 .

[tuxxy]

@Red-M Then why does it not do it for the base64 plugin?

[Red-M]

Also if the real ThisIsAnIssue could stand up please? I saw that... I have a bright red pineapple for you...

@tuxxy because this behavior affects all plugins, not just base64, I also forget where return() goes to but it should be fed into bot.message() eventually however I think it turns the sanitize to False in the keywords for legacy plugins as per foxlet.

[tuxxy]

Why do we have to maintain for legacy plugins? Why should the entire project be compromised due to maintainers who refuse to update their old plugins? This presents a clear security issue, and it needs to be fixed.

On Jun 21, 2016, at 23:02, Red_M notifications@github.com wrote:

Also if the real ThisIsAnIssue c

[Red-M]

Thats what I said...

[tuxxy]

I’m just elaborating on that point, I didn’t mean anything by it.

On Jun 21, 2016, at 23:02, Red_M notifications@github.com wrote:

Also if the real ThisIsAnIssue could stand up please? I saw that... I have a bright red pineapple for you...

@tuxxy https://github.com/tuxxy because this behavior affects all plugins, not just base64, I also forget where return() goes to but it should be fed into bot.message() eventually however I think it turns the sanitize to False in the keywords for legacy plugins as per foxlet.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227644283, or mute the thread https://github.com/notifications/unsubscribe/ADbn7qAd2FtrHPpQEFFtVHrFyCS4hC7Zks5qOMHqgaJpZM4EW3s1.

[Red-M]

If plugins really need multi-line it should return an array/list of all the lines it wants to send or use bot.message() with the string swapped for an array/list.

No, I've pretty much said that to foxlet when he reverted my commit then refused to change it back and was then forced to change it then basically kept pushing to have it there till CB got to where it is now with this problem.

[tuxxy]

I agree this needs to be fixed ASAP.

On Jun 21, 2016, at 23:10, Red_M notifications@github.com wrote:

If plugins really need multi-line it should return an array/list of all the lines it wants to send or use bot.message() with the string swapped for an array/list.

No, I've pretty much said that to foxlet when he reverted my commit then refused to change it back and was then forced to then basically kept pushing to have it there till CB got to where it is now with this problem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CloudBotIRC/CloudBot/issues/158#issuecomment-227645100, or mute the thread https://github.com/notifications/unsubscribe/ADbn7pjkKF7e9Q0tHOpUEnlz6q3wj-T5ks5qOMPAgaJpZM4EW3s1.