Skip to content

Added JSON body processor #26

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

Closed
wants to merge 3 commits into from
Closed

Added JSON body processor #26

wants to merge 3 commits into from

Conversation

urma
Copy link

@urma urma commented Jan 2, 2013

Added support for a JSON body processor. Brief description:

  • It uses yajl (http://lloyd.github.com/yajl/) for streaming JSON parsing
  • Only leaf nodes in the JSON stream will be registered under ARGS, because we currently only assign scalar values to ARGS, not JSON structures:
    { 'data': { 'a': 1, 'b': 2 } } would result in ARGS:data.a = 1 and ARGS:data.b = 2, but ARGS:data would be undefined
  • Multiple values for a given key are supported, but due to the previous mapping method, there is no way to distinguish between some scenarios:
    [ { 'data': 1 }, { 'data': 2 } ] would result in ARGS:data = 1 and ARGS:data = 2, while
    { 'data': [ 1, 2 ] } would also result in ARGS:data = 1 and ARGS:data = 2

NOTE: Once again, Makefile.in got mixed in the commit for no apparent reason; please disregard the changes in it.

@brenosilva
Copy link
Contributor

Ulisses,

Makefile.in ?
Also... looking into modsecurity-recommended.conf file .. looks like there are ` chars in the end of some rules ?

  • "id:'200001',phase:1,t:none,t:lowercase,pass,nolog,ctl:requestBodyProcessor=JSON"`    <----- here
    
  • "id:'200000',phase:1,t:none,t:lowercase,pass,nolog,ctl:requestBodyProcessor=XML"`   <---- here
    

any reason for this ?

@urma
Copy link
Author

urma commented Jan 2, 2013

Breno

Makefile.in was added again for no apparent reason, sorry about that -- can you ignore it or do I need to submit a new pull request?

Regarding modsecurity-recommended.conf -- no reason, since I used an existing modsecurity.conf in the tests I probably missed that noise in the end of the lines. Will fix right away.

@brenosilva
Copy link
Contributor

Ok. I test it tomorrow and commit.

I will let you know any feedback.

Thanks

@brenosilva
Copy link
Contributor

Ulisses,

Getting some problems to compile modsecurity with yajl lib:

apache2: Syntax error on line 208 of /etc/apache2/apache2.conf: Syntax error on line 8 of /etc/apache2/httpd.conf: Cannot load /usr/lib/apache2/modules/mod_security2.so into server: /usr/lib/apache2/modules/mod_security2.so: undefined symbol: yajl_parse

I probably means that we are missing -lyajl

So.. can you add a new build/find_yajl.m4 file to set the right @YAJL_CFLAGS@, @YAJL_LDADD@ variables into apache2/Makefile.am ?

in find_yajl.m4 you should define a CHECK_YAJL function and call it from configure.ac.

You can use the find_lua.m4 as an example.

@urma
Copy link
Author

urma commented Jan 3, 2013

Breno,

I added a check for yajl_parse in configure.ac, which on my system (Ubuntu 12.04) is enough to have the library added to CFLAGS/LIBS:

# Check for YAJL libs (for JSON body processor)
AC_SEARCH_LIBS([yajl_alloc], [yajl])

I considered creating a m4 file anyway, just for consistency, but yajl does not provide a yajl-config like pcre and libxml2. Is there an example somewhere for a library which does not provide a *-config utility? I could write some shell script to locate both headers and the library, but if there is a cleaner way of doing it, please let me know.

BTW, just as a sanity check -- do you have yajl installed? I used a package (http://packages.debian.org/stable/libdevel/libyajl-dev), which is for the 1.x version, instead of the current 2.x version (which apparently no Linux distribution is offering yet).

@brenosilva
Copy link
Contributor

Ulisses,

I installed it, both 1.x and 2.x.

Yes it is not *-config based package. Lua is not too. So find_lua.m4 file is a good example how to do this. I will try -config routines (because in some distros we have it) but also will try to find lua information without it.

It is working in you linux because probably it is installed in some default paths. But this is not a real case. Many users install libs in optional directories. We need to handle it.

Also... yajl 1.x is legacy and it is not being updated for at least 3 years :/
I installed yajl 2.x in my linux box, so looking like is is working. So we need to use the current master (2.x) code... there are som api changes so you probably need to add #ifdef hacks.

and play with

@brenosilva
Copy link
Contributor

.. and play with
#define YAJL_MAJOR 1
#define YAJL_MINOR 0
#define YAJL_MICRO 12

@urma
Copy link
Author

urma commented Jan 3, 2013

Acknowledged. I will add support for both yajl versions (1.x and 2.x) and use the find_lua.m4 script as a basis for a find_yajl.m4 script.

@brenosilva
Copy link
Contributor

Cool.

Also could you please conditionally compile it ? like we do with lua. We have WITH_LUA macro do compile it.
This is a library that could not compile in some platforms, so this feature shouldn't be mandatory.

@urma
Copy link
Author

urma commented Jan 3, 2013

Sure. I knew I would very likely face more issues with the build system than with the code itself, so thanks for the tips.

@brenosilva
Copy link
Contributor

Yes. The code looks good! I'm starting some tests.

Looks like we just need to improve the build system.

when you define your find_yajl.m4 we will have a new --with-yajl option. So the users can set it to "no" and you just will not pass -DWITH_YAJL to the compiler.

@zimmerle
Copy link
Contributor

@urma Do you mind if i continue this?

Besides this build system improvement, there is something else that should be done that you guys already know? @brenosilva?

@zimmerle
Copy link
Contributor

@urma's patches on the top of the current 'trunk' is available at:

https://github.com/zimmerle/ModSecurity/tree/urma_json_parser

Note however that it is not compiling due to some changes on the 'yajl' api. Not investigated yet. On my way to get it working.

@urma
Copy link
Author

urma commented Sep 18, 2013

@zimmerle yajl 1.x is not compatible with 2.x -- my code was originally written for 1.x because that was what Debian offered as packages. I think I never got to adding the macros to wrap around the API differences (which are minor). It should also be taken care of in the autoconf m4 files.

@zimmerle
Copy link
Contributor

zimmerle commented Oct 1, 2013

Just to keep it updated, the branch 'urma_json_parser' is working like a charm, and the branch 'urma_json_parser_with_json_collection_draft' works in the same fashion of the 'urma_json_parse' but adds the JSON inside a collection labeled JSON. They can be fetched from:

@zimmerle
Copy link
Contributor

zimmerle commented Dec 4, 2013

merged into trunk branch. target for the next release.

Thanks @urma ;)

@zimmerle zimmerle closed this Dec 4, 2013
@eoftedal
Copy link

eoftedal commented Mar 5, 2014

@zimmerle Any idea when this will be released?

@zimmerle
Copy link
Contributor

zimmerle commented Mar 5, 2014

Hi @eoftedal, the target is our next release which should happens in the upcoming weeks.

This is currently under the branch:
https://github.com/SpiderLabs/ModSecurity/tree/json_top_of_2_7_7

Before merge it we have to fix a bug reported by a users which was testing it:
http://sourceforge.net/p/mod-security/mailman/message/31966595/

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

Successfully merging this pull request may close these issues.

4 participants