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

Potential Buffer Overflow from user-controllable Array Index value #4278

Closed
kirit1193 opened this Issue Aug 31, 2017 · 15 comments

Comments

Projects
None yet
6 participants
@kirit1193

kirit1193 commented Aug 31, 2017

The clusterLoadConfig function within /redis/src/cluster.c seems to allow for a Buffer Overflow vulnerability leading from an array index being set from user-controllable input. The vulnerable code is:

direction = p[1]; /* Either '>' or '<' */
slot = atoi(argv[j]+1);
p += 3;
cn = clusterLookupNode(p);
if (!cn) {
   cn = createClusterNode(p,0);
   clusterAddNode(cn);
   }
if (direction == '>') {
   server.cluster->migrating_slots_to[slot] = cn;
} else {
   server.cluster->importing_slots_from[slot] = cn;
}

As we can see, the slot variable is receiving the output of atoi being run here: slot = atoi(argv[j]+1);

Now, argv[j] is basically the arguments of each line (stored in line[]) being put into the array using sdssplitargs for further processing.

The slot value, after being extracted is then used in the if-else block which controls the slot migration, i.e. server.cluster->migrating_slots_to[slot] = cn; and server.cluster->importing_slots_from[slot] = cn;.

It is safe to assume that a user won't try to put in invalid slot numbers in the cluster configuration file. However, an attacker with limited access to the machine would be able to trigger memory corruption issues or even potentially execute code by forcing an Array Index out of Bounds exception from happening due to invalid values of slot. There should be some validation on the value of slot and the maximum length of the migrating_slots_to and migrating_slots_from arrays.

@kirit1193

This comment has been minimized.

Show comment
Hide comment
@kirit1193

kirit1193 Sep 20, 2017

Was this confirmed as a bug that needs to be fixed?

kirit1193 commented Sep 20, 2017

Was this confirmed as a bug that needs to be fixed?

@carnil

This comment has been minimized.

Show comment
Hide comment
@carnil

carnil Oct 6, 2017

This issue has been assigned CVE-2017-15047

carnil commented Oct 6, 2017

This issue has been assigned CVE-2017-15047

@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Oct 6, 2017

Contributor
Contributor

badboy commented Oct 6, 2017

@natoscott

This comment has been minimized.

Show comment
Hide comment
@natoscott

natoscott Oct 9, 2017

FWIW, I don't think this is actually exploitable with a normal configuration of Redis, where
the cluster-config-file is being used from a default location. For example, on Fedora we'd
be creating the file in /var/lib/redis (dir directive in redis.conf) by default, which is writable
by the redis:redis user:group, which a local unprivileged user would not have access to.

IOW, an administrator would have to have changed the default settings, such that this file
is used from an unprotected location, such that its contents became "user-controllable" by
users without elevated privileges.

Anyway, I've opened PR4365 to add some bounds checking with improved diagnostics,
and to perform a more orderly shutdown of the server in this invalid-input situation.

natoscott commented Oct 9, 2017

FWIW, I don't think this is actually exploitable with a normal configuration of Redis, where
the cluster-config-file is being used from a default location. For example, on Fedora we'd
be creating the file in /var/lib/redis (dir directive in redis.conf) by default, which is writable
by the redis:redis user:group, which a local unprivileged user would not have access to.

IOW, an administrator would have to have changed the default settings, such that this file
is used from an unprotected location, such that its contents became "user-controllable" by
users without elevated privileges.

Anyway, I've opened PR4365 to add some bounds checking with improved diagnostics,
and to perform a more orderly shutdown of the server in this invalid-input situation.

@badboy

This comment has been minimized.

Show comment
Hide comment
@badboy

badboy Oct 9, 2017

Contributor

It's true that this is not really exploitable without otherwise access. Still a good idea to fix it.

Contributor

badboy commented Oct 9, 2017

It's true that this is not really exploitable without otherwise access. Still a good idea to fix it.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Oct 9, 2017

Contributor

@kirit1193 How did you find this out of interest?

Contributor

lamby commented Oct 9, 2017

@kirit1193 How did you find this out of interest?

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Oct 9, 2017

Contributor

Added in natoscott@0ba2932

All versions since 2.6.0-rc1 affected.

Contributor

lamby commented Oct 9, 2017

Added in natoscott@0ba2932

All versions since 2.6.0-rc1 affected.

@kirit1193

This comment has been minimized.

Show comment
Hide comment
@kirit1193

kirit1193 Oct 9, 2017

@lamby , I was grepping for some strings and functions in the src folder, looking for these kinds of issues in general.

kirit1193 commented Oct 9, 2017

@lamby , I was grepping for some strings and functions in the src folder, looking for these kinds of issues in general.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Oct 9, 2017

Contributor

@antirez I'd love to get an upstream blessing on the patch so distributions can roll-out updates, even if there is no new latest released version yet.

Contributor

lamby commented Oct 9, 2017

@antirez I'd love to get an upstream blessing on the patch so distributions can roll-out updates, even if there is no new latest released version yet.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Oct 12, 2017

Contributor

@antirez Ping? :)

Contributor

lamby commented Oct 12, 2017

@antirez Ping? :)

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Oct 31, 2017

Owner

Hello, I agree it's worth to fix indeed. I'm fixing it and patching all versions. However this bug will not trigger a new release of Redis because anyway we are going to have new releases soon and the bug is not critical enough, so expect Redis versions with such a fix to actually be deployed in a few weeks. Thanks for reporting, following up soon with the fix.

Owner

antirez commented Oct 31, 2017

Hello, I agree it's worth to fix indeed. I'm fixing it and patching all versions. However this bug will not trigger a new release of Redis because anyway we are going to have new releases soon and the bug is not critical enough, so expect Redis versions with such a fix to actually be deployed in a few weeks. Thanks for reporting, following up soon with the fix.

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Oct 31, 2017

Owner

Note that there is another similar bug here as well:

                start = atoi(argv[j]);
                stop = atoi(p+1);
            } else {
                start = stop = atoi(argv[j]);
            }
            while(start <= stop) clusterAddSlot(n, start++);
Owner

antirez commented Oct 31, 2017

Note that there is another similar bug here as well:

                start = atoi(argv[j]);
                stop = atoi(p+1);
            } else {
                start = stop = atoi(argv[j]);
            }
            while(start <= stop) clusterAddSlot(n, start++);

@antirez antirez closed this in ffcf7d5 Oct 31, 2017

antirez added a commit that referenced this issue Oct 31, 2017

Fix buffer overflows occurring reading redis.conf.
There was not enough sanity checking in the code loading the slots of
Redis Cluster from the nodes.conf file, this resulted into the
attacker's ability to write data at random addresses in the process
memory, by manipulating the index of the array. The bug seems
exploitable using the following techique: the config file may be altered so
that one of the nodes gets, as node ID (which is the first field inside the
structure) some data that is actually executable: then by writing this
address in selected places, this node ID part can be executed after a
jump. So it is mostly just a matter of effort in order to exploit the
bug. In practice however the issue is not very critical because the
bug requires an unprivileged user to be able to modify the Redis cluster
nodes configuration, and at the same time this should result in some
gain. However Redis normally is unprivileged as well. Yet much better to
have this fixed indeed.

Fix #4278.

antirez added a commit that referenced this issue Oct 31, 2017

Fix buffer overflows occurring reading redis.conf.
There was not enough sanity checking in the code loading the slots of
Redis Cluster from the nodes.conf file, this resulted into the
attacker's ability to write data at random addresses in the process
memory, by manipulating the index of the array. The bug seems
exploitable using the following techique: the config file may be altered so
that one of the nodes gets, as node ID (which is the first field inside the
structure) some data that is actually executable: then by writing this
address in selected places, this node ID part can be executed after a
jump. So it is mostly just a matter of effort in order to exploit the
bug. In practice however the issue is not very critical because the
bug requires an unprivileged user to be able to modify the Redis cluster
nodes configuration, and at the same time this should result in some
gain. However Redis normally is unprivileged as well. Yet much better to
have this fixed indeed.

Fix #4278.
@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Oct 31, 2017

Owner

Bug fixed in all the branches. I think the bug is exploitable, because you can basically write the address of a Cluster node structure everywhere in the process memory. So the attacker may:

  1. Modify the nodes.conf so that a node is loaded with the ID containing actually a sequence which is executable.
  2. Being the node ID almost the first field of the structure (the second actually), we could set the address in a place (like a function pointer) so that the execution of the program will jump there.

The problem with this approach is that there is the ctime field as the first member of the structure, so there are 8 initial bytes the user cannot completely control, however probably there is some way to still exploit the bug, for instance by also using the other clusterAddSlot() call to selectively alter the last byte bits of the return address or alike. Did not investigate very carefully but in general it looks exploitable with efforts.

Owner

antirez commented Oct 31, 2017

Bug fixed in all the branches. I think the bug is exploitable, because you can basically write the address of a Cluster node structure everywhere in the process memory. So the attacker may:

  1. Modify the nodes.conf so that a node is loaded with the ID containing actually a sequence which is executable.
  2. Being the node ID almost the first field of the structure (the second actually), we could set the address in a place (like a function pointer) so that the execution of the program will jump there.

The problem with this approach is that there is the ctime field as the first member of the structure, so there are 8 initial bytes the user cannot completely control, however probably there is some way to still exploit the bug, for instance by also using the other clusterAddSlot() call to selectively alter the last byte bits of the return address or alike. Did not investigate very carefully but in general it looks exploitable with efforts.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Oct 31, 2017

Contributor

Thanks @antirez. I had already uploaded the previous patch/PR to Debian in 4.0.2-4 but I've replaced it with ffcf7d5 in 4.0.2-5.

Contributor

lamby commented Oct 31, 2017

Thanks @antirez. I had already uploaded the previous patch/PR to Debian in 4.0.2-4 but I've replaced it with ffcf7d5 in 4.0.2-5.

@kirit1193

This comment has been minimized.

Show comment
Hide comment
@kirit1193

kirit1193 Nov 2, 2017

I think the bug is exploitable, because you can basically write the address of a Cluster node structure everywhere in the process memory.

This is what I had in mind when I was reporting this as well. Just didn't have a malicious nodes.conf file which would be able to achieve this.

kirit1193 commented Nov 2, 2017

I think the bug is exploitable, because you can basically write the address of a Cluster node structure everywhere in the process memory.

This is what I had in mind when I was reporting this as well. Just didn't have a malicious nodes.conf file which would be able to achieve this.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Jan 13, 2018

Fix buffer overflows occurring reading redis.conf.
There was not enough sanity checking in the code loading the slots of
Redis Cluster from the nodes.conf file, this resulted into the
attacker's ability to write data at random addresses in the process
memory, by manipulating the index of the array. The bug seems
exploitable using the following techique: the config file may be altered so
that one of the nodes gets, as node ID (which is the first field inside the
structure) some data that is actually executable: then by writing this
address in selected places, this node ID part can be executed after a
jump. So it is mostly just a matter of effort in order to exploit the
bug. In practice however the issue is not very critical because the
bug requires an unprivileged user to be able to modify the Redis cluster
nodes configuration, and at the same time this should result in some
gain. However Redis normally is unprivileged as well. Yet much better to
have this fixed indeed.

Fix #4278.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Jan 13, 2018

Fix buffer overflows occurring reading redis.conf.
There was not enough sanity checking in the code loading the slots of
Redis Cluster from the nodes.conf file, this resulted into the
attacker's ability to write data at random addresses in the process
memory, by manipulating the index of the array. The bug seems
exploitable using the following techique: the config file may be altered so
that one of the nodes gets, as node ID (which is the first field inside the
structure) some data that is actually executable: then by writing this
address in selected places, this node ID part can be executed after a
jump. So it is mostly just a matter of effort in order to exploit the
bug. In practice however the issue is not very critical because the
bug requires an unprivileged user to be able to modify the Redis cluster
nodes configuration, and at the same time this should result in some
gain. However Redis normally is unprivileged as well. Yet much better to
have this fixed indeed.

Fix #4278.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Jan 13, 2018

Fix buffer overflows occurring reading redis.conf.
There was not enough sanity checking in the code loading the slots of
Redis Cluster from the nodes.conf file, this resulted into the
attacker's ability to write data at random addresses in the process
memory, by manipulating the index of the array. The bug seems
exploitable using the following techique: the config file may be altered so
that one of the nodes gets, as node ID (which is the first field inside the
structure) some data that is actually executable: then by writing this
address in selected places, this node ID part can be executed after a
jump. So it is mostly just a matter of effort in order to exploit the
bug. In practice however the issue is not very critical because the
bug requires an unprivileged user to be able to modify the Redis cluster
nodes configuration, and at the same time this should result in some
gain. However Redis normally is unprivileged as well. Yet much better to
have this fixed indeed.

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