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

[Vuln] Potential arbitrary-memory-log in ds_is_in_list due to uninit varaible on stack #2780

Closed
Cossack9989 opened this issue Mar 16, 2022 · 9 comments
Assignees
Milestone

Comments

@Cossack9989
Copy link

Sorry to issue this publicly, but when I mailed to security@opensips.org, googlemail told me that "Your message wasn't delivered to security@opensips.org because the address couldn't be found"

Detail

In ds_is_in_list, the first variable val has no initialization before https://github.com/OpenSIPS/opensips/blob/master/modules/dispatcher/dispatch.c#L2327 ,and if variable ip is not valid(str2ip(ip)==NULL && str2ip6(ip==NULL)), the function will access val.rs.len, val.rs.s by LM_ERR.
For that reason, in some cases/samples, users are allowed to use "ds_is_in_list" cmd, then an attacker may be able to pollute the stack before calling ds_is_in_list in order to control the uninitialized val.rs.s to arbitrary memory address and val.rs.len to arbitrary integer, resulting in content of arbitrary memory address to be logged (such as passwords/cookies/sessions/....).

/* Checks, if the request (sip_msg *_m) comes from a host in a set
 * (set-id or -1 for all sets)
 */
int ds_is_in_list(struct sip_msg *_m, str *_ip, int port, int set,
                  ds_partition_t *partition, int active_only, str *pattern_s)
{
        pv_value_t val;
        ds_set_p list;
        struct ip_addr *ip;
        int_str avp_val;
        int j,k;
        char *pattern = NULL;

        if (!(ip = str2ip(_ip)) && !(ip = str2ip6(_ip))) {
                LM_ERR("IP val is not IP <%.*s>\n",val.rs.len,val.rs.s);
                return -1;
        }

        if (pattern_s) {
                pattern = pkg_malloc(pattern_s->len + 1);
                if (!pattern) {
                        LM_ERR("oom for pattern!\n");
                        return -1;
                }
                memcpy(pattern, pattern_s->s, pattern_s->len);
                pattern[pattern_s->len] = '\0';
        }

        memset(&val, 0, sizeof(pv_value_t));
        val.flags = PV_VAL_INT|PV_TYPE_INT;

        /* access ds data under reader's lock */
....

Suggestion

Initialize val before accessing val.rs

@liviuchircu liviuchircu self-assigned this Mar 16, 2022
@liviuchircu liviuchircu added this to the 3.1.9 milestone Mar 16, 2022
@Cossack9989
Copy link
Author

BTW, will a GHSA/CVE be assigned to this issue? @liviuchircu

liviuchircu added a commit that referenced this issue Mar 16, 2022
Many thanks to @Cossack9989 for the report!

Fixes #2780

(cherry picked from commit e2f13d3)
liviuchircu added a commit that referenced this issue Mar 16, 2022
Many thanks to @Cossack9989 for the report!

Fixes #2780

(cherry picked from commit e2f13d3)
@liviuchircu
Copy link
Member

liviuchircu commented Mar 16, 2022

Thank you for the report, @Cossack9989! I see this bug as a minor problem, as in the overwhelming majority of use-cases, ds_is_in_list() will be simply invoked as ds_is_in_list($si, ...), which will guarantee a correctly-formatted IP address. Moreover, if the script writer were to use $rd instead of $si, thus making use of the Request-URI domain, they would run into the crash very early, during the platform testing phase.

@Cossack9989
Copy link
Author

Although ds_is_in_list() will be simply invoked as ds_is_in_list($si, ...) in the overwhelming majority of use-cases, but no one can guarantee that an incorrectly-formatted ip will never be passed to ds_is_in_list() for various reasons caused by users(e.g in dispatcher's README, ds_is_in_list($rd,...) has appeared twice). What's more, if the invalid-ip code block will never be accessed, why design it?

For that reason, I think this issue is actually a vulnerability and deserves a GHSA/CVE(to remind anybody using dispatcher not to misuse ds_is_in_list). @liviuchircu

@Cossack9989
Copy link
Author

What's more, if an incorrectly-formatted ip is passed to ds_is_in_list(), it may lead to secret information leak to log due to uninitialized val. From the effect, it can indeed be called a security vulnerability. @liviuchircu

@liviuchircu
Copy link
Member

@Cossack9989 - ultimately, you are right. If a single user can be affected by the vulnerability, then it is real and we cannot leave anything to chance. I have opened up a security advisory on the bug and gave you access to it.

So please do a full review of the security implications, as well as my own notes on the vulnerability and complete these with your own feedback. Once that is done, we will most likely release this advisory to the public along with the Security Audit update.

@Cossack9989
Copy link
Author

Cossack9989 commented Mar 16, 2022

@liviuchircu I think that the patch e2f13d374 works. Now you can release the sa to the public.
BTW, could you request a CVE for the issue?

@liviuchircu
Copy link
Member

I think a GHSA is more than enough, for this little bug. However, you have full permission to reserve, write and request an entire CVE ID and article with the same information I wrote in the advisory.

@Cossack9989
Copy link
Author

When will the sa be released to the public?

@liviuchircu
Copy link
Member

Hi, @Cossack9989! It's been a while, but the OpenSIPS Security Audit findings have just been fully disclosed -- more info on this blog. So I took the opportunity to also publish this Advisory and have also requested a CVE for it. Let's see how it goes!

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

No branches or pull requests

2 participants