SoftwareBrothers / adminjs-objection

MIT License
1 stars 2 forks source link

SQLinjection? #6

Closed JanSmolko closed 1 year ago

JanSmolko commented 1 year ago

lib/utils/convert-filter.js

...
} else if (property.type() === 'string') {
  // Should be safe: https://github.com/knex/documentation/issues/73#issuecomment-572482153
  qb.where(raw(`lower(${path})`), operators.like, `%${String(value).toLowerCase()}%`);
} else {
...

using raw() with user defined parameter is the problem. I can force path value with query params. I looked at code used in adminjs and it shouldnt be exploitable with their default code. It will fail because of your check of property.type() , BUT because you can write you own handlers/whatever in adminjs, we can mess something up. Maybe there is even some way to exploit it now.

To give an example: if your code would just be convert-filters.ts

export const convertFilter = (
  qb: QueryBuilder<Model, Model[]>,
  originalFilter: Filter,
): QueryBuilder<Model, Model[]> => {
  if (!originalFilter) return qb;

  const { filters } = originalFilter;

  Object.values(filters).forEach((filter) => {
    const { path, value } = filter;

    qb.where(raw(path), operators.eq, value as string);
  });

  return qb;
};

and I had in my adminjs project this in handler: accountsResource.ts

...
customAction: {
  ...
  search: async (request, response, context) => {
   // same code as in adminjs repo for search action, I will just make second argument empty object
    ...
    const records = await resource.find(filter, {}, context)
    ...
  }
}

and I would call ..../actions/search/x?searchProperty="x"%20OR%201=1%20OR%20lower(label) I would be able to run anything in DB with help of searchProperty.

Of course this library shouldn't allow it. I don't know how to fix that and keep same functionality at the same time.

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 2.0.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: