Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

protect against non-base64 images #2912

Closed
wants to merge 6 commits into from

4 participants

@Carreau
Owner

Be sure not to receive non-base64 string.
If so just don't display them as it might break.

IPython/frontend/html/notebook/static/js/outputarea.js
@@ -424,8 +424,13 @@ var IPython = (function (IPython) {
}, 250);
}
+ var base64re = /^([A-Za-z0-9+]{4})*([A-Za-z0-9+]{2}==|[A-Za-z0-9+]{3}=)?$/
@minrk Owner
minrk added a note

I don't think this expression is correct. It misses '/', the 64th symbol in b64.

@Carreau Owner
Carreau added a note

True, I probably had a bad cache when testing.It does not take newline into account neither. Will fix later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Carreau
Owner

Fixed. I didn't find a good way to treat newline, I went with .split('\n').join('').
New RegEx confuses vim syntax highlighting. And I'm astonished to see that my spell checker recognize RegEx.

@minrk
Owner

splitting / rejoining all images? This is getting to be an expensive operation. Surely js has multiline regex support.

@Carreau
Owner

Surely js has multiline regex support

Yes on most browser.
I'll let you imagine on which one(s) it does not behave.
There are ugly workaround using [\s\S] and [\\n\\r] but I dindn't found any clear answer for cross browser/os behavior for those.

@ellisonbg
Owner

@Carreau can you post a notebook (maybe a new examples directory examples/security?) that has a hacked data-url that this branch removes properly?

@ellisonbg
Owner

@Carreau I too am a bit concerned about splitting imagines on new lines (performance). Is IE the problem but Chrome/Safari/FF OK?

@ellisonbg
Owner

I should have noted the reason I would like to see an example notebook is so we can later do a security audit to make sure these things have not crept back in.

@ellisonbg
Owner

@Carreau any progress on adding an example notebook to help us test this? This has been inactive for >2 months now. I think it is mostly ready right? Should we merge as is, or wait until you add an example?

@minrk
Owner
@Carreau Carreau was assigned
@Carreau
Owner

Still havent found time to search for a cross browser way to handle multiline regex.

I'm not confortable in shipping a security fix that's say "It mostly should prevent arbitrary code execution, except when it doesn't"

@ellisonbg
Owner

How specifically can this test fail?

IPython/frontend/html/notebook/static/js/outputarea.js
((5 lines not shown))
OutputArea.prototype.append_png = function (png, element) {
+ if( base64re.test(png.split('\n').join('')) != true ){
@minrk Owner
minrk added a note

if this has to be done, at the very least, it can be with a single regexp substitution. Can we check if that's faster?

@Carreau Owner
Carreau added a note

I think the problem is the same, there is an inconsistency in dealing with newline in javascript across browser.
But, we could try the otherway arround which is checking wether the string contain any caracter other than A-Za-z0-9+/= and fail in this case. So we will have false positive (non base64 string that pass the test), but cannot have 'dangerous caracters' like <&>... and this will probably be also faster because the regex matching will return at the first bad char.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
eteq and others added some commits
@eteq eteq Added clickable icon to collapse pager
Closes #3196
11526d8
@eteq eteq changed ESC to trigger collapsing of the pager in notebook 494e7ba
@eteq eteq Collapse -> Close in button description as suggested by @fperez 1a1e6e5
@Carreau Carreau protect against non-base64 images
use simple, but with false negative expression

Use a simple expression to detect non base64 strings.
this should work much faster and reliably between browser and with/
without newlines. But a few non base64 string could go through.

Those that goes through should be harmless as they will only be unvalid
base64 sequences or aphanum caracters plu +,/ and =
01e90dd
@Carreau
Owner

Simplify the logic. This add false negative (but which are harmless).
This should be X-browser, and faster.

@Carreau
Owner

Except the rogue commits that I will remove (and rebase) what do you think of this way of preventing rogue base 64 ?

@ellisonbg
Owner

@Carreau I played around a bit with trying to embed scripts in data URIs but couldn't figure it out. Could you post a notebook somewhere that demonstrates how this works. Then we can figure out if this PR fixes the issue. Thanks.

@Carreau
Owner

Hum, something seem to have change in javascript/browser. Quote seem to be escaped when you use attr('src',string_containing_quotes), I'll have to play with escaping to see if i can make it work again. But nbviewer shoudl be still vulnerable, but it is less important,

@Carreau
Owner

Closing for now as seem to be fixed by itself...

@Carreau Carreau closed this
@Carreau Carreau deleted the Carreau:rogue-base64 branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 29, 2013
  1. @eteq

    Added clickable icon to collapse pager

    eteq authored
    Closes #3196
  2. @eteq
  3. @eteq
Commits on May 5, 2013
  1. @Carreau

    protect against non-base64 images

    Carreau authored
    use simple, but with false negative expression
    
    Use a simple expression to detect non base64 strings.
    this should work much faster and reliably between browser and with/
    without newlines. But a few non base64 string could go through.
    
    Those that goes through should be harmless as they will only be unvalid
    base64 sequences or aphanum caracters plu +,/ and =
  2. @Carreau
  3. @Carreau
This page is out of date. Refresh to see the latest.
View
1  IPython/frontend/html/notebook/static/js/notebook.js
@@ -136,6 +136,7 @@ var IPython = (function (IPython) {
} else if (event.which === key.ESC) {
// Intercept escape at highest level to avoid closing
// websocket connection with firefox
+ that.element.trigger('collapse_pager');
event.preventDefault();
} else if (event.which === key.SHIFT) {
// ignore shift keydown
View
12 IPython/frontend/html/notebook/static/js/outputarea.js
@@ -428,8 +428,16 @@ var IPython = (function (IPython) {
}, 250);
};
+ OutputArea.notbase64re = /[^A-Za-z0-9+/=\n]/;
OutputArea.prototype.append_png = function (png, md, element) {
+ if( OutputArea.notbase64re.test(png) == true ){
+ // we are sure this is **not** a base 64 encoded string.
+ // but a few other string could go through.
+ // but those should be harmless
+ console.log('Cowardly refusing to insert non-base 64 png');
+ return
+ }
var toinsert = $("<div/>").addClass("output_subarea output_png");
var img = $("<img/>").attr('src','data:image/png;base64,'+png);
if (md['height']) {
@@ -445,6 +453,10 @@ var IPython = (function (IPython) {
OutputArea.prototype.append_jpeg = function (jpeg, md, element) {
+ if( OutputArea.notbase64re.test(jpeg) == true ){
+ console.log('Cowardly refusing to insert non-base 64 jpeg');
+ return
+ }
var toinsert = $("<div/>").addClass("output_subarea output_jpeg");
var img = $("<img/>").attr('src','data:image/jpeg;base64,'+jpeg);
if (md['height']) {
View
14 IPython/frontend/html/notebook/static/js/pager.js
@@ -48,14 +48,24 @@ var IPython = (function (IPython) {
var that = this;
this.pager_button_area.append(
$('<a>').attr('role', "button")
- .attr('title',"open the pager in an external window")
+ .attr('title',"Open the pager in an external window")
.addClass('ui-button')
.click(function(){that.detach()})
- .attr('style','position: absolute; right: 10px;')
+ .attr('style','position: absolute; right: 20px;')
.append(
$('<span>').addClass("ui-icon ui-icon-extlink")
)
)
+ this.pager_button_area.append(
+ $('<a>').attr('role', "button")
+ .attr('title',"Close the pager")
+ .addClass('ui-button')
+ .click(function(){that.collapse()})
+ .attr('style','position: absolute; right: 5px;')
+ .append(
+ $('<span>').addClass("ui-icon ui-icon-close")
+ )
+ )
};
Pager.prototype.style = function () {
Something went wrong with that request. Please try again.