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

Crash while adding channels to the web admin #528

Closed
RussellB28 opened this issue Apr 13, 2014 · 9 comments
Closed

Crash while adding channels to the web admin #528

RussellB28 opened this issue Apr 13, 2014 · 9 comments

Comments

@RussellB28
Copy link

Adding an already existing channel to a user/network via web admin causes a crash if the channel name isn't prefixed with '#' (i.e you already have a channel '#potato' but you try to add 'potato' without the '#' prefix).

This only appears to happen if the channel is already in the list and not if you are adding a new channel that isn't already in the list.

Adding the following lines below "CString sChanName = WebSock.GetParam("name").Trim_n();" in modules/webadmin.cpp fixes this problem (although its probably not the best way of fixing it)

if(sChanName.substr(0, 1) != "#")
{
        sChanName = "#" + sChanName;
}
psychon added a commit that referenced this issue Apr 14, 2014
The CChan constructor makes sure that the channel name begins with a valid
channel prefix. Thus, this could change the name of the resulting channel.

When you edited an irc network which already had a channel "#foo", were
connected to IRC (so ZNC knows which prefixes are valid) and added a channel
"foo", this would lead to a problem:

Webadmin checks and sees that there is no channel "foo" yet. Webadmin creates a
new CChan instance for "foo". The CChan constructor notices that "f" is not a
valid channel prefix and instead calls itself "#foo". Then,
CIRCNetwork::AddChan() would see that this channel already exists, delete the
given channel and return false.

However, webadmin didn't check this result and would continue changing settings
on an already destroyed CChan instance.

Fix this by checking if the channel exists after CChan had its chance to mess
with the channel name. Also handle failures from CIRCNetwork::AddChan().

Fixes #528.

Signed-off-by: Uli Schlachter <psychon@znc.in>
(cherry picked from commit 5e6e3be)
@seblu
Copy link

seblu commented Apr 15, 2014

A security release could be done?

@DarthGandalf
Copy link
Member

@seblu yeah

@seblu
Copy link

seblu commented Apr 17, 2014

soon?

amyreese pushed a commit to amyreese/znc that referenced this issue May 5, 2014
The CChan constructor makes sure that the channel name begins with a valid
channel prefix. Thus, this could change the name of the resulting channel.

When you edited an irc network which already had a channel "#foo", were
connected to IRC (so ZNC knows which prefixes are valid) and added a channel
"foo", this would lead to a problem:

Webadmin checks and sees that there is no channel "foo" yet. Webadmin creates a
new CChan instance for "foo". The CChan constructor notices that "f" is not a
valid channel prefix and instead calls itself "#foo". Then,
CIRCNetwork::AddChan() would see that this channel already exists, delete the
given channel and return false.

However, webadmin didn't check this result and would continue changing settings
on an already destroyed CChan instance.

Fix this by checking if the channel exists after CChan had its chance to mess
with the channel name. Also handle failures from CIRCNetwork::AddChan().

Fixes znc#528.

Signed-off-by: Uli Schlachter <psychon@znc.in>
@seblu
Copy link

seblu commented May 5, 2014

I will backport the patch in Archlinux as there is still no security release.

@DarthGandalf
Copy link
Member

Done

@Xe
Copy link
Contributor

Xe commented Dec 18, 2014

This has been assigned CVE-2014-9403.

@Mikaela
Copy link
Contributor

Mikaela commented Dec 18, 2014

1.6 will probably be released in near future.

@DarthGandalf
Copy link
Member

@Mikaela why 1.6 matters? 1.4 is fixed already

@Mikaela
Copy link
Contributor

Mikaela commented Dec 18, 2014

@DarthGandalf I thought this was something that is coming with 1.6 because of the new comment.

DarthGandalf added a commit that referenced this issue Oct 30, 2015
DarthGandalf added a commit that referenced this issue Oct 30, 2015
The previous fix (5e6e3be) left a possibility to use-after-delete,
though it has been much harder to accidentally trigger.

If AddChan(pChan) fails, it deletes pChan, so the new crash was
happening during showing of error message.

Test for this is at master branch: 9777a1a

Thanks to https://scan.coverity.com/ for pointing at this.
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

No branches or pull requests

5 participants