Skip to main content
Added example 'exploit'
Source Link
user45891
  • 630
  • 6
  • 10

I'll mostly judge the actual parsing part, and not that much the boilerplate stuff around it

Why explode the file twice?

$element should be called $elements, and this loop won't really work as expected:

foreach ($element as $element)

There can be multiple selectors for one block:

h2, b {
    ...

Currently that counts as only one in your code.

What about comments?

/* This is for bold text*/ 
b {

gives /* This is for bold text*/ LINEBREAK b as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually, why not split $a_name[1]?

Counting to < count($styles) is enough (that also saves you the trimming); PHP indexes arrays from 0 on.

Then you never check for errors. If it doesn't have any { in it, it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow.

And so on for basically every step you do.

Read the specs when doing this. E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "/../../../../../etc/passwd". You prepend some hash and some paths so your $this->newFileName becomes /var/www/example.com/HASH/../../../../etc/passwd. The ../ means go one directory up so your end up with just /etc/passwd.

And then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing, some information from it can be read.

I'll mostly judge the actual parsing part, and not that much the boilerplate stuff around it

Why explode the file twice?

$element should be called $elements, and this loop won't really work as expected:

foreach ($element as $element)

There can be multiple selectors for one block:

h2, b {
    ...

Currently that counts as only one in your code.

What about comments?

/* This is for bold text*/ 
b {

gives /* This is for bold text*/ LINEBREAK b as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually, why not split $a_name[1]?

Counting to < count($styles) is enough (that also saves you the trimming); PHP indexes arrays from 0 on.

Then you never check for errors. If it doesn't have any { in it, it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow.

And so on for basically every step you do.

Read the specs when doing this. E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing, some information from it can be read.

I'll mostly judge the actual parsing part, and not that much the boilerplate stuff around it

Why explode the file twice?

$element should be called $elements, and this loop won't really work as expected:

foreach ($element as $element)

There can be multiple selectors for one block:

h2, b {
    ...

Currently that counts as only one in your code.

What about comments?

/* This is for bold text*/ 
b {

gives /* This is for bold text*/ LINEBREAK b as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually, why not split $a_name[1]?

Counting to < count($styles) is enough (that also saves you the trimming); PHP indexes arrays from 0 on.

Then you never check for errors. If it doesn't have any { in it, it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow.

And so on for basically every step you do.

Read the specs when doing this. E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "/../../../../../etc/passwd". You prepend some hash and some paths so your $this->newFileName becomes /var/www/example.com/HASH/../../../../etc/passwd. The ../ means go one directory up so your end up with just /etc/passwd.

And then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing, some information from it can be read.

added 14 characters in body
Source Link
Alex L
  • 5.8k
  • 2
  • 26
  • 69

I'll mostly judge the actual parsing part, and not that much the boilerplate stuff around it

Why explode the file twice?
$element

$element should be called $elements$elements, and this loop won't really work as expected:

foreach ($element as $element)

There can be multiple selectors for one block:

h2, b {
    ...

Currently that counts as only one in your code.

What about comments?

/* This is for bold text*/ 
b {

gives "/* This is for bold text*/ LINEBREAK b"/* This is for bold text*/ LINEBREAK b as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {{? Actually, why not split $a_name1$a_name[1]?

Counting to < count($styles)< count($styles) is enough (that also saves you the trimming) -; PHP indexes arrayarrays from 0 on.

Then you never check for errors. If it doesn't have any '{'{ in it, it can't be a CSS file, no need to parse further. Or what if your $a_key_value$a_key_value has something but 2 elements? That's an error and should be noted somehow. And

And so on for basically every step you do. But read

Read the specs when doing this:. E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents()file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing, some information from it can be read.

I'll mostly judge the actual parsing part and not that much the boilerplate stuff around it

Why explode the file twice?
$element should be called $elements and this loop won't really work as expected

foreach ($element as $element)

There can be multiple selectors for one block

h2, b {
    ...

Currently that counts as only one in your code

What about comments?

/* This is for bold text*/ 
b {

gives "/* This is for bold text*/ LINEBREAK b" as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually why not split $a_name1?

Counting to < count($styles) is enough (that also saves you the trimming) - PHP indexes array from 0 on.

Then you never check for errors. If it doesn't have any '{' in it it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow. And so on for basically every step you do. But read the specs when doing this: E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing some information from it can be read.

I'll mostly judge the actual parsing part, and not that much the boilerplate stuff around it

Why explode the file twice?

$element should be called $elements, and this loop won't really work as expected:

foreach ($element as $element)

There can be multiple selectors for one block:

h2, b {
    ...

Currently that counts as only one in your code.

What about comments?

/* This is for bold text*/ 
b {

gives /* This is for bold text*/ LINEBREAK b as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually, why not split $a_name[1]?

Counting to < count($styles) is enough (that also saves you the trimming); PHP indexes arrays from 0 on.

Then you never check for errors. If it doesn't have any { in it, it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow.

And so on for basically every step you do.

Read the specs when doing this. E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing, some information from it can be read.

Added part about selectors
Source Link
user45891
  • 630
  • 6
  • 10

I'll mostly judge the actual parsing part and not that much the boilerplate stuff around it

Why explode the file twice?
$element should be called $elements and this loop won't really work as expected

foreach ($element as $element)

There can be multiple selectors for one block

h2, b {
    ...

Currently that counts as only one in your code

What about comments?

/* This is for bold text*/ 
b {

gives "/* This is for bold text*/ LINEBREAK b" as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually why not split $a_name[1]$a_name1?

Counting to < count($styles) is enough (that also saves you the trimming) - PHP indexes array from 0 on.

Then you never check for errors. If it doesn't have any '{' in it it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow. And so on for basically every step you do. But read the specs when doing this: E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing some information from it can be read.

I'll mostly judge the actual parsing part and not that much the boilerplate stuff around it

Why explode the file twice?
$element should be called $elements and this loop won't really work as expected

foreach ($element as $element)

There can be multiple selectors for one block

h2, b {
    ...

Currently that counts as only one in your code

What about comments?

/* This is for bold text*/ 
b {

gives "/* This is for bold text*/ LINEBREAK b" as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually why not split $a_name[1]?

Counting to < count($styles) is enough (that also saves you the trimming) - PHP indexes array from 0 on.

Then you never check for errors. If it doesn't have any '{' in it it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow. And so on for basically every step you do.

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing some information from it can be read.

I'll mostly judge the actual parsing part and not that much the boilerplate stuff around it

Why explode the file twice?
$element should be called $elements and this loop won't really work as expected

foreach ($element as $element)

There can be multiple selectors for one block

h2, b {
    ...

Currently that counts as only one in your code

What about comments?

/* This is for bold text*/ 
b {

gives "/* This is for bold text*/ LINEBREAK b" as name in your example. Certainly not right.

When you try to remove the element name from the key:values what happens if I put a space before the {? Actually why not split $a_name1?

Counting to < count($styles) is enough (that also saves you the trimming) - PHP indexes array from 0 on.

Then you never check for errors. If it doesn't have any '{' in it it can't be a CSS file, no need to parse further. Or what if your $a_key_value has something but 2 elements? That's an error and should be noted somehow. And so on for basically every step you do. But read the specs when doing this: E.g. did you know about the many types of selectors (I only knew 4 of those and when I had written this parser I would totally have rejected E + F and similar)

For some security: An attacker could use your file_get_contents() call to open any file on your server. All he needs to do is set the file name to "../../../../../etc/passwd" and then you try to parse that as CSS. It's highly unlikely that it will contain valid CSS but as you currently don't try to stop the parsing some information from it can be read.

Source Link
user45891
  • 630
  • 6
  • 10
Loading